share

Pull Requests – Fluch und Segen

Die Zusammenarbeit bei OpenSource-Projekten auf GitHub oder GitLab ist großartig. Ich freue mich immer wie ein Schneekönig, wenn jemand eine Erweiterung für eines unserer OpenSource-Projekte macht und über einen Pull Request zum Mergen übergibt.

TOXID ist so über die Jahre gewachsen, dass es inziwschen in großen Enterprise-Projekten zum Einsatz kommt, das cleartmp-Modul aus dem Kochbuch ist erweitert worden und kommt viel zum Einsatz und für viele andere Module bekommen wir Feedback.

Was jedoch –  und damit zum „Aber“ – das mergen, also das Zusammenführend des Codes unglaublich schwer macht, ist das automatische Ändern von Code-Style durch die Entwicklungsumgebungen.

Was passiert, ist, dass große Codeblöcke und teilweise ganze Dateien als geändert markiert werden, weil die IDE Methoden umsortiert, andere Einrückungn hat, etc.

Man sitzt also vor einem riesigen Block Änderungen und muss sehr mühsam suchen, was denn nun wirklich geändert wurde. Das kostet Zeit, Nerven und ist nicht mal so eben nebenher zu machen.

Wie sieht ein guter Pull Request aus?

  1. Es werden neue Funktionen hinzugefügt
  2. Sind Code-Style-Änderungen unbedingt nötig, werden die in einem separaten Commit gemacht, der entsprechend markiert ist.
  3. Umsortieren kompletter Methoden findet nicht statt.
  4. Code-Style-Änderungen gehen immer in Richtung des betroffenen Projekts.

Inzwischen gibt es ja einen de Facto Standard für Codestyle, den alle großen Projekte die wir einsetzen verfolgen: PSR-2

Wir werden daher alle Module auf PSR-2 wechseln. Doch seht es uns nach, wenn das nicht immer klappt, weil wir unseren alten Style noch gewöhnt sind.

Um Energie zu sparen, werden wir bei folgenden Änderungen nur sehr selten mergen:

  • Pull Requests die NUR Codestyle ändern. Das ist kein Update bei allen Usern wert!
  • Pull Requests, die Funktions-Änderung und Code-Style-Änderungen an bestehendem in einem Commit enthalten.

Pull Requests, die Codestyleänderungen an bestehendem Code enthalten die nicht PSR-2 sind.

Ich freue mich, auf die nächsten gemeinsamen Projekte und gerne eine anregende Diskussion!

Joscha Krug ist Gründer und Geschäftsführer der marmalade GmbH aus Magdeburg. Mit seinem stetig wachsenden Team realisiert er schon seit 2009 E-Commerce Projekte mit OXID eShop und Shopware. Er ist Autor der beiden OXID Bücher, erschienen bei o'Reilly.

4 Kommentare zu:

Pull Requests – Fluch und Segen

  • LeSpocky

    Die meisten OpenSource-Projekte haben einen gewissen Coding-Style und es ist üblich, dass upstream fordert, dass der auch eingehalten wird. D.h. für die Leute, die Pull Requests schicken, sich dem anzupassen und ggf. nachzuarbeiten. Meiner Meinung nach kann das auch nur so funktionieren. Von upstream zu verlangen alles selbst anzupassen, wäre unfair und der Aufwand würde auf der falschen Seite passieren. Wenn Ihr hier ganz klar ansagt, was genau der geforderter Style ist und jener sauber dokumentiert ist: umso besser, das erleichtert es Leuten, die was beitragen wollen. 🙂

  • Robin Lehrmann

    Also heißt das, wenn man in einem OpenSource-Projekt auf Github bei euch, Code optimiert um die Performance und die Verständlichkeit zu verbessern wird dieser nicht gemerged bzw erst nach längerer Zeit? Und was ist, wenn bei einem Modul der CodeSniffer Fehler ausgibt? Beim ocb_cleartmp Modul ist das der Fall. Wenn ich an der Logik des Moduls nichts ändere und die Funktionalität bestehen bleibt spricht doch eigentlich nichts gegen einen merge oder?

    Meiner Meinung nach kann man mit Test-Driven-Design bei OpenSource-Projekten am meisten erreichen. Vielleicht wäre es ja mal eine Idee für eure Module PHPUnit-Tests einzuführen um somit pull-request einfacher und schneller mergen zu können. Bei Github-Projekten ist es doch auch möglich CI-Apps als hook zu hinterlegen. Da kommt am besten das hier in Frage: https://travis-ci.org/
    Ich weiß nicht ob es möglich ist aber vielleicht kann man ja auch den CodeSniffer als hook irgendwie bei github integrieren. Somit hätte man wieder einen Automatismus, der einem Arbeit erspart und Code Qualität garantiert.

  • Joscha Krug Artikelautor

    Also grundsätzlich, wie gesagt, sind Pull Requests ja das, was man haben möchte. Selbstverständlich schaue ich mir jeden davon in der Regel innerhalb von Stunden an. Sofern es irgendwie geht, merge ich dann auch direkt. Das kann ich aber nur, wenn ich sehe, was geändert wurde und weiß, welche Auswirkungen das hat.

    Kann ich das nicht, muss ich den Code manuell prüfen und in Demoshops testen. Ja, UnitTests würden das ganze deutlich vereinfachen. Aber eine Garantie hat man damit auch nicht, und das Filesystem zu Mocken ist kein Zuckerschlecken.

    Du bist natürlich herzlich eingeladen, dich hier auszutoben und die Tests zu schreiben. Uns fehlt im Tagesgeschäft häufig die Zeit, das für bestehende Module nachzuziehen. Unsere Kernmodule wie OXSEARCH sind bereits mit Tests versehen. Ich weiß also sehr wohl um die Vorteile.

    Grundlegend, um das nochmal klar zu sagen, ist es eben wichtig, sich beim Coden Gedanken darüber zu machen, was es bedeutet, das, was man macht zu mergen und ob es sinnvoll ist, dem Reviewer die Zeit aufzuzwingen, weil die eigene IDE eben mal was umstellt, weil sie das für besser hält. Macht man das bewusst, kann man es in einem separaten Commit tun.

  • Alfred Bez

    Moin, der Artikel ist zwar schon ein weniger älter, ich möchte aber auf einige Dinge hinweisen:
    1. Whitespace kann man in den Diffs ganz einfach ignorieren:
    git diff -w
    oder auf Github folgenden Parameter an die URL anhängen: ?w=1
    also z.B. https://github.com/marmaladeDE/csvexporter/pull/3/files?w=1
    2. Die Datei .editorconfig nimmt einige Formatierungen des Codes automatisch vor, dazu muss allerdings das entsprechende Plugin in der IDE/im Texteditor installiert sein. siehe dazu auch: http://editorconfig.org/, im Wiki finden sich einige große Projekte, die bereits eine .editorconfig in ihren Projekten zur Verfügung stellen: https://github.com/editorconfig/editorconfig/wiki/Projects-Using-EditorConfig
    3. Entwickler können mit relativ wenig Aufwand dafür sorgen, dass sie „automatisch“ richtigen Codestyle abliefern, indem sie sich die IDE entsprechende einrichten. Dazu gibt es in vielen großen Projekten auch Hilfestellungen, wie z.B. hier für OXID: https://github.com/OXID-eSales/coding_standards
    4. Wenn ich als Entwickler Änderungen zum commit vorbereite, dann vermeide ich, alle Änderungen auf einmal zu stagen, sondern nutze git add -p, um damit nur das zu committen, was auch wirklich in diesen commit gehört. Wenn ich mehrere Dinge am Code geändert habe, dann mache ich dafür auch mehrere commits.

  • Kommentare geschlossen.