Automatische Code Formatierung

Ich versuche es nochmal mit ein paar Argumenten und Kompromissvorschlägen…

Niemand schaut das Zeile für Zeile an und das ist auch gar nicht notwendig. clang-format sollte ausgereift genug sein um nur in seltenen Ausnahmefällen Fehler einzubauen. Wenn dich das so sehr beunruhigt, dann hat @laszloh oben schon mal einen möglichen Kompromissvorschlag gemacht, bei dem in der CI für einen PR jeweils nur der geänderte Code von clang-format geprüft wird.

Damit würde sich die Umformatierung auf viele kleine Schritte verlagern wobei das unschöne daran vor allem wäre, dass es unterschiedlich formatierten Bereiche im Code gibt und der Aufwand in Summe größer ist, als wenn alles auf einmal umformatiert würde.

Nochmal: Die letzte Umformatierung war eben nur die halbe Miete. Es wird keine „ständigen“ Umformatierungen mehr geben, wenn man einmal den Style definiert, umsetzt und maschinell prüft. Momentan ist er undefiniert und nicht prüfbar.

Wie oben schon gesagt ist der Output von clang-format offenbar für Tab-Size: 4 ausgelegt. Damit sieht es besser aus wie auf deinen Screenshots.

Für dich scheint es wichtig zu sein, dass die Ausrichtung der Kommentare bündig ist. Das kann ich grundsätzlich nachvollziehen. Das führt aber automatisch auch dazu dass die Zeilen ewig lang werden und lange Zeilen (> 100 Zeichen) gelten allgemein als schlecht lesbar und sind zu vermeiden.

Vielleicht ist das grundlegende Problem ja nicht die Formatierung durch clang-format, sondern das Layout an sich. Zum Dokumentieren der Makros wäre es m. E. viel schöner wenn man den gängigen, Doxygen-kompatiblen Stil wählt:

/**
 * @brief Used as dummy for RC522
 */
#define RST_PIN	  99

/**
 * @brief GPIO for chip select (RFID)
 */
#define RFID_CS	  21

/**
 * @brief GPIO for master out slave in (RFID)
 */
#define RFID_MOSI 23

Mich stört hier die Floskel „Kein Kompromiss“. Ich weiß, dass das nicht deine Grundhaltung ist, weil du in der Vergangenheit immer wieder Kompromisse eingegangen bist. Wenn es aber bei diesem Thema „dein letztes Wort ist“ finde ich das etwas kurzsichtig. Die Formatierung soll doch denen dienen, die mit dem Code arbeiten und das bist mittlerweile ja nicht mehr du alleine.

Akzeptiert, aber vielleicht findet sich ja doch noch ein Kompromiss oder eine Möglichkeit dich zu überzeugen. :wink:

Das ist genau das Problem. Du hast Ansprüche an die Formatierung, die aber niemand genau kennt (da nicht dokumentiert) und die teilweise von gängigen Standards abweichen. Zudem sind diese Anforderungen nicht maschinell prüfbar, was bei Änderungen aus der Community sowohl beim Autor als auch beim Maintainer zu unnötigem Mehraufwand führt (auch wenn du der Meinung bist, das das nicht nennenswert ist).

Ich denke das hatten wir an anderer Stelle schon. Der wesentliche Grund warum man Code für den Preprocessor und für den Compiler nicht gleich einrückt ist, dass beide nichts miteinander zu tun haben. Details und ein passendes Beispiel siehe: vim - Indentation of preprocessor statements in C - Stack Overflow.

2 „Gefällt mir“

Ich werfe noch ein persönliches Argument für eine automatisierte Formatierung in den Raum:

Formatieren nervt. Ich will mir keine Gedanken um Formatierung machen. Ja, ich will dass es ordentlich aussieht, aber keine Zeit damit verbringen, irgendwelche Sachen einzurücken. Zum Glück nehmen die allermeisten Editoren einem da ohnehin viel Arbeit ab.

Vielleicht findet sich ja doch noch eine Konfiguration, die die problematischen Stellen besser formatiert. Die Idee mit dem doxygen-Kommentarstil finde ich auch sehr gut.

Zu den 6k Zeilen Code-Änderung: ich sehe schon auch ein Problem mit der Nachvollziehbarkeit, da kann ich Torsten verstehen. Wer sagt denn, dass kein Schadcode eingeschleust wurde? Nicht, dass ich das unterstelle.

Du kannst ein Diff ohne whitespace Beachtung erzeugen, das sind dann deutlich weniger Zeilen und einfach zu validieren:

3 „Gefällt mir“

Sehr guter Hinweis, danke! Da es um Formatierung geht machen Whitespace-Changes (Einrückung, Ausrichtung, etc.), die für den Compiler allesamt irrelevant sind hier tatsächlich den Großteil aus. Von den >6k geänderten LOC bleiben ohne diese am Ende effektiv nur 41 files changed, 220 insertions(+), 335 deletions(-) (siehe git diff --stat --ignore-all-space laszloh/feature/code-formatting 48cb9d17d800aa81f87ce50177e379dcad87e702) was problemlos zu überblicken ist.

1 „Gefällt mir“

Und das Problem mit git blame lässt sich ggf. auch über die git config lösen: bestimmte commits können für git blame ignoriert werden

2 „Gefällt mir“

Danke, das kannte ich noch nicht. Das ist sehr nützlich und wäre eine brauchbare Lösung für das oben von Torsten beschriebene Problem. Für alle die nicht so tief drin stecken: git blame ermöglicht das Anzeigen der letzten Änderung zu einer bestimmten Codezeile.

Laut https://medium.com/codex/how-to-introduce-a-code-formatter-without-messing-up-git-history-4a16bd074c10 wird das auch von GitHub und VSCode (via GitLens Extension) unterstützt.

1 „Gefällt mir“

Um allen Spekulationen und Befürchtungen bezüglich der automatischen Änderungen von clang-format vorzubeugen habe ich mir mal die Binaries (ohne Änderungen an den Settings) vor und nach dem Commit angeschaut.

Ergebnis: Bis auf 4 Bytes sind die Symbole in Flash und RAM (ermittelt via elf-size-analyze) identisch. Die einzige Änderung ist in FindNsID() aus lib/nvsdump/src/nvsDump.h (die von clang-format nicht einmal angefasst wird) und kommt vermutlich daher, dass in der Web.cpp das #include <nvsDump.h> an einer anderen Stelle steht und ein jetzt zuvor eingebundener Header in irgendeiner Weise Auswirkungen auf FindNsID() hat. Wer noch eine bessere Erklärung hat, gerne melden. :wink:

Man kann also guten Gewissens sagen, dass die automatischen Formatierungs-Änderungen (außer vielleicht bis auf die oben erwähnte Ausnahme, die sicher erklärbar oder vermeidbar ist) keinerlei Auswirkungen auf die resultierende Firmware und damit auf die Funktion haben.

1 „Gefällt mir“

Mei, dann stellt’s halt um. Scheint ja irgendwie elementar wichtig zu sein.
Aber: Die Settings-Files bleiben so, wie sie sind. Und zwar alle.

Ich wollte dir damit nicht auf die Nerven gehen. Falls das der Fall war, sorry!
Wenn wir es umsetzen, dann wäre es doch gut wenn du und/oder @tueddy sich ein bisschen damit identifizieren können. Ich hoffe daher dass du die Vorteile auch siehst und nicht nur einlenkst um uns ruhig zu stellen. :wink:

Ja, das halte ich auch erst mal für sinnvoll.

Um noch auf @tueddy oben einzugehen:

Automatisch abgelehnt wird natürlich nichts. Es wird dem Ersteller nur eine Rückmeldung gegeben ob die Formatierung passt oder ob noch Korrekturbedarf besteht. Wenn jemand mit den Informationen nichts anfangen kann, gibt es immer die Möglichkeit dass jemand erfahreneres mit Erklärungen oder Korrekturen zur Seite steht.

Abgeschreckt werden sollte natürlich niemand, aber ich kann mir auch nicht vorstellen, dass die hier diskutierten Änderungen dazu beitragen würden.

@laszloh Hast du noch Motivation übrig um den PR weiter zu verfolgen? Zu tun wäre vermutlich folgendes:

  • Rebase
  • Ausschließen der Settings-Header
  • Einführen einer .git-blame-ignore-revs Datei und einem zugehörigen Eintrag für GitLens in .vscode/settings.json
  • Abschnitt in der README mit Informationen zum Code-Style und zu clang-format anlegen
1 „Gefällt mir“

Nee, mir fehlt für ESPuino derweil einfach der Nerv wenn ich ehrlich bin.
Ich bin in zwei hiesigen Vereinen im Vorstand (regelmäßige Meetings), helfe auch sonst öfter mal bei Vereinskram, hocke für gewöhnlich mind. 200 km/Woche auf dem Rad, betreibe nen Webserver mit ner dreistelligen Anzahl an Domains und nicht zuletzt habe ich noch Familie (inkl. zwei Kinder). Soweit normal und unspannend. Was mich gerade aber dekompensieren lässt ist einerseits, dass es wegen der Kinder im Mai enorm viele Termine gibt und ich gerade meine PV-Anlage auf dem Dach installiere. Letztgenanntes beschäftigt mich jetzt schon ein paar Wochen, da es da halt einiges zu wissen gibt. Auch die Ausführung ist echt viel Arbeit, im Laufe des Juni wird das hoffentlich abgeschlossen sein (ich habe halt nicht dauernd dafür Zeit und will auch nicht bei Hitze auf dem Dach hocken) und dann habe ich auch wieder mehr Zeit.
In den letzten Jahren fand ich es ja immer schade, wenn es im Sommer rund um ESPuino so ruhig wurde. Aktuell brauche ich die Zeit einfach anderweitig und deswegen bin ich hier auch ziemlich inaktiv und teste derweil nicht mal :woman_shrugging:. ESPuino-Anfragen gibt es aktuell sehr wenig - das kommt mir gelegen. Demletzt, das möchte ich an dieser Stelle jedoch mal erwähnen, war sogar mal eine Anfrage aus Saudi-Arabien dabei (!).

Naja, ich sage mal im Endeffekt sind Falschformatierungen bisher nicht ausgeartet. Mir persönlich macht’s auch nix aus, wenn ich Sachen von Hand einrücken muss. Ich habe so ein paar Vorlieben, wie ich Kram gerne eingerückt hätte, aber speziell was Kommentare etc (abgesehen von den Settings) angeht, muss das für mich auch nicht 100 % uniform sein. Insofern hat das, das bin ich ehrlich, für mich erstmal wenig Mehrwert. Solch gravierende (vom Umfang) Codeänderungen haben aber massig Impact für mich, da Probleme oft direkt bei mir und gar nicht hier im Forum landen. Das werde ich künftig anders steuern.

Was ihr immer bedenken müsst: Die breite Basis der ESPuino-Nutzer ist gar nicht daran interessiert, hier irgendwelchen Code anzupassen (und das ist völligst ok so). Aber die Eintrittsschwelle in ESPuino ist eh schon nicht so fürchterlich niedrig - ich will sie nicht noch höher legen. Insofern will ich ESPuino nicht noch komplizierter machen und daher möchte ich die Settings-Files (als Kontaktpunkte quasi zu den Usern) so beibehalten.

1 „Gefällt mir“

Das ist ja völlig ok. Ich denke das kennt jeder. Zeit und Motivation sind stark begrenzte Ressourcen. Aber gerade deshalb ist es doch gut wenn es in der Community ein paar Leute gibt, die immer mal wieder was übernehmen und z. B. in der Software auch Hilfe bei der Fehlersuche, etc. beisteuern können. Das ist ja das gute an einem Hobby-Projekt: es gibt keinerlei Zwang oder so (außer vielleicht der eigene Ehrgeiz :wink: ).

Ich denke nicht, dass du dich für alles was am Code geändert wird verantwortlich fühlen musst. Das sollte über das Forum ganz gut funktionieren im Ernstfall jemanden zu finden, der sich um ein Problem kümmert.

Da sind wir ganz auf einer Wellenlänge. Das Ziel sollte sein, ESPuino für unerfahrene Nutzer zugänglicher zu machen und die Software so zu gestalten, dass möglichst viele gut damit klar kommen. Ehrlich gesagt sind die Settings-Header da auch nicht das gelbe vom Ei für jemanden der noch nie mit Quellcode gearbeitet hat. Daher ja auch mein Ansatz mit Kconfig und einem sich daraus ergebenden Einstellungs-Menü. Da steckt noch viel Potential drin die Schwelle niedriger zu legen.

4 „Gefällt mir“

@laszloh Ich habe gesehen, dass du hier schon mit den Korrekturen angefangen hast. Wie siehst du die Chancen, dass du das voll zu Ende bringen kannst?

Ich fände es auch sehr schön, diese Funktion noch drin zu haben. Bin es einfach nicht mehr gewohnt meinen Code selbst zu formatieren :slight_smile: . Das macht auch direkt weniger Spaß am Code in diesem Projekt zu arbeiten.
Ist dabei völlig egal ob es produktiver Code oder einfach nur Codeschnipsel fürs debugging sind.

Stimme dir da zu. @laszloh : wenn du keine Lust mehr darauf hast, bist du dabei wenn jemand anderes das voll auf diese Art abschließt?

1 „Gefällt mir“

Ich hab das Thema jetzt nochmal aufgegriffen, weil ich es nach wie vor für wichtig halte. Da @laszloh nicht mehr reagiert hat, habe ich seinen Branch nochmal überarbeitet und auf den aktuellen Stand aufgesetzt.

Vielleicht ist das jetzt ja eine günstige Gelegenheit die Formatierung anzugehen bevor neue, größere Änderungen im (aktuell „leeren“) Dev-Branch landen.

Hier also der PR mit Bitte um Feedback: Code Formatting via `clang-format` by fschrempf · Pull Request #264 · biologist79/ESPuino · GitHub.

1 „Gefällt mir“

Mehrfach hier kommuniziert, also ich mache da keine Review:

grafik

Heißt das du schaust dir den ganzen PR nicht an, weil du inhaltlich nicht einverstanden bist, oder du möchtest nicht alle Formatierungsänderungen prüfen?

Letzteres ist nachvollziehbar und auch überhaupt nicht notwendig. Die automatischen Formatierungsänderungen sind separat in einem Commit. Dieser Commit kann und muss auch nicht vollständig gereviewed werden. Wir können problemlos davon ausgehen, dass clang-format korrekt arbeitet.

Wenn die Regeln nach denen formatiert wird i. O. sind und jeder mit dem Stil halbwegs leben kann (haben wir weiter oben ja schon ausgiebig debattiert), dann passt das.

Ich habe weiter oben ja auch schon nachgewiesen, dass die Formatierungsänderungen keine Auswirkungen auf das resultierende Binary haben.

Ich starte eine Review zunächst über die geänderten Dateien in Github, das ist hier einfach unmöglich weil zu viele Änderungen die ich nicht überblicken kann:

grafik

Habe daher Deinen Branch ausgecheckt, getestet & er kompiliert wie gewohnt, so weit so gut. Allerdings empfinde ich die Lesbarkeit als eine Verschlechterung zum bestehenden Codestyle. Z.B. ist das Einrücken der conditional defines unglücklich (Und davon haben wir viele!), Mal wird’s eingerückt, mal wieder nicht.

Hier nicht:

Hier doch:

1 „Gefällt mir“

clang bzw der fomatter schreibt alle #DEFINE an den Anfang der Zeile, und rückt die nicht ein, egal was vorher ist…

Das sorgt imho für eine bessere Trenung von „normalen“ ifs (die werden zur Laufzeit ausgewertet) und #IFDEF die zur Compile Zeit eine Auswertung bekommen.

1 „Gefällt mir“

So, ich hab’s jetzt gemerged.
Und ich habe auch gleich ingores für git blame eingetragen.

Damit ist das Thema jetzt hoffentlich durch.

3 „Gefällt mir“

Was machen wir jetzt eigentlich mit .editorconfig?