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. Here is a process that has worked well in my experience: - 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. - Commits to this branch can be as granular as the programmer wants, generally the more frequent the commits the better - When the programmer is ready for feedback, they should create a pull request between this branch and the main branch - they could post the URL of the pull request to IRC to encourage feedback - It's fine for a programmer to request feedback before the feature is complete, but they should note this in the title of the pull request - eg. "DO_NOT_MERGE" - For code review, the most useful tab is "Files changed" - which shows all differences between the feature branch and the main branch (individual commits are irrelevant here) - comments can be added here - Once the feature is complete and the review feedback has been read and perhaps acted upon by the programmer working on the feature, it should be merged 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? Ian. -- Ian Clarke Founder, The Freenet Project Email: i...@freenetproject.org _______________________________________________ Devl mailing list Devl@freenetproject.org https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
