Closed AlastairHolmes closed 9 months ago
As discussed the first thing we want to do is find a set of axiomatic principles. I think it will be neccessary that these principles are documented as more than just a phrase. But have proper explanation, and examples/case studies (To ensure people understand and buyin).
I will collect some ideas myself of course, but it would be helpful for ideas to be gathered by you guys, and possibly recorded here before the meeting.
Think collaboratively: https://google.github.io/eng-practices/review/developer/handling-comments.html
Some good general principles we might want to take some inspiration on: https://github.com/subspace/subspace/blob/main/CONTRIBUTING.md#commits
From above on meaningful commits: Commits should tell a logical step by step story of the changes and ideally be individually meaningful. Use squashing and rebasing during development, reorder commits if it makes more sense that way, there is great tooling out there for these kinds of modifications.
Reviewer should be able to go through commits one by one and understand what is being changed, with non-trivial changes it is often very hard to review the final diff, so going through commits helps a lot. For this commits should be well-structured and there should be a reasonable number of them. 70 commits for 100 lines or changes is most likely bad, same with one commit that changes many thousands of lines of code in various places at once (lock files are a frequent and notable exception).
If you work on a branch and after a few commits notice that API introduced earlier doesn't work well or need to revert some changes, amend original commit instead of creating a new one much later, since reviewer going through individual commits will likely have a comment about something that was already fixed and will simply have to spend more time reviewing changes back and forth.
Different kinds of changes should ideally be in different commits if not pull requests entirely. For instance if you want to move something and change, move in one commit so reviewer can have a quick look without thinking about it too much and apply actual changes in a separate commit. Same with file renaming, if you rename file and change it significantly in the same commit it'll make reviewer's life very difficult and likely convince Git that one file was deleted and another file added, which breaks tooling for tracking file changes across renames.
It is sometimes the case that some refactoring needs to be done during feature development, try to extract those refactoring commits you have just done into a separate branch, submit it for review and rebase your working branch on refactoring branch so that the changes are clearly separated and easier to review as an example.
An issue for tracking and discussing the initial development of our practices (Also see #2086).