So nachdem ich hier ein wenig still herum geworden ist, habe ich mal eine Branch für Clang erstellt:
Hier könnt ihr anschauen, wie der Code nach der Formatierung aussehen würde.
Folgende inhaltliche Änderungen sind auch mit drin:
Clang format check bei push & PR
PIO build run bei push und PR
Der PIO build ist mit Caches und parallel (jedes Target wird einzeln gebaut). Damit brauchen wir im Schnitt 5 Minuten um alle zu bauen (CPU Zeit gleich, ca 20 Minuten → ~3 min / Target). Die Idee habe ich von Tasmota übernommen .
Kommentare und Vorschläge sind willkommen (ihr könnt auch gerne mit Test-PR’s gegen die Branche die PR-Checks ausprobieren). Wenn es so passt, mache ich eine PR für die Änderung auf.
Wenn ich das richtig sehe wird hier nur beim Push auf dev und bei PRs gegen dev getriggert. Wäre es nicht sinnvoll auf allen Branches zu triggern (wie bisher)? Dann läuft der Build auch z. B. bei einem Feature-Branch (ohne PR) und auf dem master.
Und bei den Trigger-Pfaden: Wäre es nicht sinnvoller statt allen Dateiendungen, die entsprechenden Verzeichnisse anzugeben, also src/*, html/*, etc.
strategy:
fail-fast: true
Das fail-fast bedeutet ja, dass beim ersten fehlgeschlagenen Job abgebrochen wird. Oft will man ja aber doch wissen, welche Jobs fehlschlagen und welche auch nicht. Daher hätte ich das eher deaktiviert.
Den Branch master sah zu testen sah ich als nicht wichtig an, da der Workflow ja sein sollte PR → dev → master. Damit sollte alles was in den Master einfließt schon sauber sein. Aber an Feature branches habe ich nicht gedacht. Ich schaue ob es ein Exclude branch oder so gibt. Sonst nehme ich es einfach raus.
Die Trigger-Pfade passe ich an, die hatte ich einfach von Tasomta copy&pasted ohne groß zu hinterfragen. Muss auch gestehen, ich bin kein Meister in den Github Actions .
Hm, ja macht beiden Sinn. Ich hatte die action als go-nogo Test für die PR konzipiert, d.h. mit der Aussage, wenn eines ausfällt brauchen wir nicht weiter zu schauen. Auch unterscheiden sich die Targets bei uns fast nur im Pinout, somit ist es sehr wahrscheinlich dass nach dem Ersten auch die anderen ausfallen werden.
Für die Erleichterung der Fehlersuche macht es aber Sinn auch die anderen duchrlaufen zu lassen. Verbrauchst halt im schlechtesten Fall extra Rechenzeit (was wir als public Repo nicht bezahlen müssen, zum Glück ).
Ich auch nicht. Ich bin eher mit GitLab CI vertraut…
Ja, kann man auch so sehen und wäre für die Verifizierung auf dem dev oder im PR ausreichend. Wenn ich hingegen ein Feature gegen ein einzelnes Target entwickle ist es halt ganz nützlich die CI dann die anderen Targets testen zu lassen.
Achtung sobald dieser in der dev ist, wird das bei alle anderen PRs zu einiges an Arbeit führen. Aus dem Grund habe ich es vorerst als Draft eingestellt. Ihr könnt gerne mit dieser PR warten bis #221 und #236 gemergt sind, danach aktualisiere ich es auf die Dev Branch.
Es wird einfach so ziemlich alles umgerückt - wer soll das denn überblicken? Machst du da Regressionstests auf allen Plattformen, um das zu überprüfen?
Dann werden Dinge, die einfach gut menschenlesbar eingerückt sind, einfach sonstwie zusammengestaucht. Das geht schon im großen Bitfeld des AudioPlayers los. Sieht man auch schön bei den Tasks.
Ganz ehrlich: Ich habe mir den Kram so eingerückt, damit man das gut lesen kann. Ich habe ein Problem damit, dass das sonstwie zurechtgeschoben wird, nur um irgendwelchen Autoformattern gerecht zu werden.
Vor ein paar Wochen wurde quasi alles neu von bliepp formatiert wegen editorconf. Was mich daran schon genervt hat ist die Tatsache, dass wenn man in VSC mit dem Mauszeiger in eine Zeile gegangen ist, dann sieht man immer direkt, wer das zuletzt geändert hat (Mouseover). Daraus ergibt sich für mich üblicherweise automatisch direkt ein Kontext, warum das angepasst wurde. Das war im Anschluss alles weg. Und das wird jetzt schon wieder passieren.
Bisher habe ich die Diskussion um die Formatierung nicht sehr verfolgt. Ich habe nicht viel Ahnung vom Programmieren in C o.ä. ,und kann mir nur hier und da was zusammenfrickeln. Ich komme aus der Assembler-Ecke und das ist schon sehr lange her. Damals habe ich auch immer versucht es übersichtlich zu halten. Deshalb bin ich auch gegen das neue Format weil es gerade für Anfänger wie mich viel unübersichtlicher wird.
Nun gut, damit ist das Thema für mich abgeschlossen.
Eine automatische Formatierung die 1:1 die aktuelle widerspiegelt wird es nicht geben. Uncrustify wäre vielleicht am nächsten dran, aber der muss extern von VSCode installiert werden. Plus die EInstellungen sind viele, da kann sich gern jemand andere rumspielen.
Wenn es euch interessiert, könnt ihr den build workflow von hier übernehmen, der funktioniert auch unabhängig von dem Rest. Bringt neben der Geschwindigkeit auch den Vorteil mit sich, dass neue Targets übersichtlicher eingetragen werden können
Eine Möglichkeit wäre es über Uncrustify oder clang eine Formatierung von neuen / geänderten Dateien in einer PR zu erzwingen. Wie das machbar wäre geht aber über mein wissen von Github Actions weit hinaus.
Vielleicht lässt man es einfach so, wie es aktuell ist. @laszloh Sorry wenn ich dich da harsch angegangen bin, das geht nicht persönlich gegen dich. Aber irgendwie gefällt mir das so nicht.
Muss ehrlich sagen, dass ich von deiner Reaktion ziemlich überrascht bin.
Das ganze war ein konstruktiver Beitrag zu einem ernsthaften Problem, das wann immer man es angehen wird zu goßen Änderungen führen würde. Anders geht es nicht wenn man das dauerhaft lösen möchte. Aber dann besser einmal sauber als immer mal wieder zerstückelt.
Ich akzeptiere die Assage: „Wir lassen das wie es ist“ völlig und verstehe das auch durch die damit verbunden Auswirkungen, die Art der Reaktion finde ich aber diesem Thema nicht angemessen.
Dir will sicher keiner die Entscheidungshoheit absprechen, aber das kann man auch anders abschließen zumal das ganze Thema ja erst auf die Formulierung des Problems von dir hin gestartet wurde…
Wieso sollten sich denn durch die Vereinheitlichung der Formatierung Regressionen ergeben? Im Gegenteil: die einheitliche und geprüfte Formatierung würde mit sehr großer Wahrscheinlichkeit wesentlich zum Verhindern von Regressionen in Zukunft beitragen.
Das sieht in GitHub schlimmer aus als es tatsächlich ist. Wenn ich in VSCode die vom Clang-Formatter umformatierte AudioPlayer.h anschaue und die Einrückung auf Tab Size: 4 stelle, sieht das schon ganz in Ordnung aus.
Es geht nicht darum dem Auto-Formatter gerecht zu werden. Es geht darum eine einheitliche Formtierung zu haben, die einen Kompromiss zwischen gut lesbar und maschinell überprüfbar bietet. Wenn man das nicht hat, wird man in Zukunft weiterhin eine erhebliche Menge an Zeit brauchen um manuell sicherzustellen, dass die Formatierung passt und der Code lesbar bleibt. Wenn man das jetzt einmal umsetzt muss man sich zu dem Thema in Zukunft so gut wie keine Gedanken mehr machen und kann die Zeit stattdessen zur eigentlichen Entwicklung der Software verwenden. Das ist ein meiner Meinung nach ein entscheidender Vorteil.
Ja, das ist richtig, aber auch hier gilt: Die Vorteile durch eine einmalige, großflächige Änderung überwiegen einfach bei weitem.
Ich finde es sehr schade, dass das „NACK“ von Torsten erst jetzt kommt. @laszloh hatte ja extra die Diskussion angestoßen und den PR sogar noch angekündigt. Da wäre es schon schön gewesen wenn von @biologist oder @tueddy früher ein Feedback gekommen wäre um ihm unnötige Arbeit zu ersparen.
Das kannst du machen, aber das löst das eigentliche Problem nicht und es ist meiner Meinung nach vollkommen unnötige Arbeit. Jedes mal wenn jemanden einen PR macht und die Code-Formatierung vermurkst, dann muss jemand den Autor darauf hinweisen, oder selbst von Hand Korrekturen vornehmen. Mit der automatischen Prüfung brauchen die Maintainer keinen Finger krumm machen, solange nicht der grüne Haken von der CI da ist und der Autor bekommt direkt die Rückmeldung was verbessert werden muss.
Komme gerade zurück vom Spanier (der heißt übrigens „ESPuino’s“) und es ist wohl schon behandelt.
Also wenn ich die Änderungen überfliege verschlechtert sich die Lesbarkeit deutlich zum bestehenden Code. Auch finde ich das automatische Ablehnen eines Code-Beitrags bedenklich. Ein Einsteiger mit weniger (Code-Style / Github) Erfahrungen traut sich womöglich gar nicht mehr hier etwas beizutragen…
Hey wir sind hier ein Hobbyprojekt!
Das habe ich noch nicht ganz geblickt, mache mich aber mal schlau.
Schönen Abend Euch!
Da sind 6k an Code verändert. Also ich schaue mir das nicht Zeile für Zeile an. Wer stellt denn sicher, dass das jetzt alles noch funktionell ist und weiß, dass hier keine Fehler passiert sind? Wir hatten das demletzt erst mit der Umformatierung und ich möchte nicht ständig das halbe Projekt umgebaut haben.
Das darfst du gerne so sehen.
Ich habe das aber lieber genau so formatiert, wie ich das gerne hätte. Kein Kompromiss. Und wenn ich dazu halt hinten raus 2-3 TAB drücken muss, dann nehme ich das so hin. Und wenn dann an einer Stelle die Kommentar mit zwei Tabs eingerückt ist und woanders mit deren drei, dann geht davon die Welt auch nicht unter.
Ja, akzeptiere ich als Kritik, wobei ich @tueddy hier in Schutz nehme - dem stelle ich mir alleine.
Ich habe das irgendwie so ein bisschen befürchtet, dass das in eine Richtung läuft, die mir am Ende nicht gefällt. Aber ich hab’s laufen lassen, um es mir doch einfach mal anzusehen und dem Ganzen eine Chance zu geben. Letztlich ist es allerdings viel invasiver, als ich dachte. Es kommt auch echt selten vor, dass ich mein Veto einlege und ich habe euch hier in den letzten Wochen wahrlich „machen lassen“. Aber diese Anpassung geht mir jetzt zu weit und da nehme ich mir an der Stelle das Recht raus zu sagen, dass ich das so nicht möchte.
Das stört mich weniger, als ständig Code zu sehen, der nicht so formatiert ist, wie ich das gerne hätte. Und wie gesagt: Es ist auch wahrlich keine Arbeit, die irgendwie ausartet. Ich gehe auch keinem Entwickler wegen z.B. einzelnen Klammern auf den Sack und es sind auch keinen komplexen Ansprüche, die ich hier stelle. Ja, das Einrücken von Compiler-Direktiven im Code ist sonst unüblich, aber ich finde, dass es die Lesbarkeit verbessert.
Ich habe die Arbeit als Maintainer vor einigen Wochen übernommen & freue mich auf zukünftige Erweiterungen & Verbesserungen & Bugfixes. Noch kann ich ich nicht jeden PR gleich kommentieren. Das ist schon aufgrund der Änderungen heftig was hier reingeworfen wurde, da kannst Du nicht erwarten hier gleich ein Klack/No-Go für was auch immer zu bekommen.
Natürlich stelle ich mich auch jeder Kritik und bin für Vorschläge offen.
So, erstmals Sorry, dass ich erst jetzt wieder reagiere. Ich habe ein wenig Abstand zu dem Thema gebraucht.
Der Punkt, der mich am meisten geärgert hat ist eine fehlende Reaktion vorab. Ich hatte die Branch die ich für die PR vorbereitet habe vor 3 Tagen gepostet nachdem das Thema schon fast ein Monat offen war, wo auch alle Commits schon drin waren. Hätte ich da schon ein „So nicht“ bekommen, so hätte ich halt nicht noch weitere 4 Abende Arbeit in den Sand gesetzt (nachdem in das Thema schon ca 1 Woche Abende rein geflossen sind).
Plus ich mag es nicht, wenn einer einfach ein „So nicht“ postet, da frag ich mich dann halt, „wie den?“.
@biologist Ich habe (vielleicht fälschlich) angenommen, dass ich im ersten Post schon darauf hingewiesen habe, dass die Änderung stark invasiv wird. Damit war ich doch recht von deiner Reaktion negativ überrascht.
TL;DR Entschuldigung angenommen .
Was ich mir für die Zukunft wünsche, ist bei solchen Diskussionen eine frühzeitige Info, ob etwas grundsätzlich gar nicht angenommen wird. ESPuino ist ein Hobby, aber halt nicht mein Einziges. Ich verschwende meine Zeit sehr ungern und hier hab ich das Gefühl die Zeit völlig verschwendet zu haben.
.
.
).
Habe fail-fast rausgenommen. Branch ist aktualisiert.
