Am Samstag, 21. März 2015, 12:45:39 schrieb Ian Clarke: > Talking to a few people, I think our current approach to code review is > problematic.
> For example, I've been told that some people are arguing that commits are > too granular, and need to be combined to make code review easier. This is a > mistake, there is nothing wrong with very granular commits, just as there > is nothing wrong with more verbose code if it helps clarity. Pull requests > should be used for code review, not individual commits. The number of > individual commits should be irrelevant. > > It sounds like people are trying to use commits for code review, whereas > they should be using Github pull requests. For huge changes we are using commits as a second level hierarchy: When the diff is too big to understand on its own, then the commits have to form an easy to follow story which allows understanding the change step by step. Different from a company where people have similar time budgets (between 20 and 50 hours per week), we have a vast disconnect of available time between paid and unpaid developers (between 2 and 30 hours per week) and the unpaid developers often cannot get an uninterrupted streak of coding time of more than 2 hours. As such they unpaid devs cannot just sit down for 6 hours and read through a huge diff to understand it in its entirety. They need the diff more structured. But doing this structuring requires an understanding of the whole change, so it cannot be done effectively by an unpaid reviewer. To solve that dilemma, the pull-request must be structured in a way which makes it feasible to review by unpaid contributors. If it is done by an unpaid developer, chances are that the pull request is already small enough to be understandable by an unpaid reviewer. The closed pull-requests include many examples where that worked well: https://github.com/freenet/fred/pulls?q=is%3Apr+is%3Aclosed If it is done by a paid developer, chances are that the changes are so extensive, that they cannot be understood in an hour. So the raw diff is effectively not reviewable by unpaid developers. By structuring the pull-request in commits, it becomes possible to understand it in several separate sessions. > - For any isolatable feature or bugfix, create a new branch just for > that feature or bug request (perhaps put the bug id # in the name of the > branch). *Do not combine multiple features or bugfixes into a single > branch.* If it can be merged independently, it should have it's own > branch. We get into a problem with this, when the changes get too extensive without being ready for merging into a release. > - Commits to this branch can be as granular as the programmer wants, > generally the more frequent the commits the better > The person contributing the code is responsible for asking for and > incorporating feedback, but they control the process, the process should > not control them. So, for example, in some cases the programmer might > decide that a code review is unnecessary. That will be their call. Better > to trust human judgement over a rigid process. > > Thoughts? Your scheme provides fast development of individual features, but does not provide information spread within the group: Only one person has seen the current version of the code. Longterm this needs to a situation where no one can understand the whole code well enough to change it efficiently, which in turn leads to pseudo-ownership over certain sub-sections of the code. This can be OK in a company, where developers can muster the time to read into new sections if someone quits. It doesn’t work in a free software community. Our contributing guidelines show the goals of code review in the project: Code review helps improve code quality, ensures that multiple people know the codebase, serves as a defense against introducing malicious code, and makes it infeasable to pressure people into contributing malicious code. Its goal is producing software that is readable, correct, and sufficiently efficient. — https://github.com/freenet/fred/pull/338/files - good code - shared knowledge of the code - securing contributors against pressure Best wishes, Arne -- Celebrate with ye beauty and gather yer friends for a Pirate Party! → http://1w6.org/english/flyerbook-rules#pirate-party ←
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ Devl mailing list Devl@freenetproject.org https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
