From ff0861f99a289cf1ba4bbe339e30a62f71c5590e Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Sun, 24 Nov 2024 18:29:58 -0800 Subject: [PATCH] docs: code review guidelines and tips (#4532) Signed-off-by: Larry Gritz --- CONTRIBUTING.md | 2 + docs/dev/CodeReview.md | 190 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 192 insertions(+) create mode 100644 docs/dev/CodeReview.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fb8cfc78b5..d3c9d673dc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -267,6 +267,8 @@ accepted. It happens to all of us. 1. After approval, one of the senior developers (with commit approval to the official main repository) will merge your fixes into the main branch. +Please see the [Code Review](docs/dev/CodeReview.md) document for more +explanaton of the code review process. Coding Style ------------ diff --git a/docs/dev/CodeReview.md b/docs/dev/CodeReview.md new file mode 100644 index 0000000000..1a82c1b9ea --- /dev/null +++ b/docs/dev/CodeReview.md @@ -0,0 +1,190 @@ +Code Review Procedures +====================== + +### Why do we code review? + +Code review is a process where someone other than the author of a patch +examines the proposed changes and approves or critiques them. The main +benefits are to: + +- Encourage submitters to ensure that their changes are well thought out, + tested, and documented so that their intent and wisdom will be clear to + others. +- Directly find shortcomings in or suggest improvements to the proposed + changes, and ensure that the changes are consistent with the project's best + practices. +- Minimize the amount of the code base that has only been seen or is only + understood by one person. +- Improve security and robustness of the code base by assuring that at least + two people (submitter and reviewer) agree that every patch is reasonable and + safe. + +### Reviewer guidelines -- what you should be looking for + +Code review is not difficult, and it doesn't require you to be a senior +developer or a domain expert. Even if you don't particularly understand that +region of the code and are not a domain expert in the feature being changed, +you can still provide a helpful second pair of eyes to look for obvious red +flags and give an adequate review: + +- This may seem too obvious to say explicitly, but a big part of review is + just looking at the PR and seeing that it doesn't appear to deface, + purposely/obviously break, or introduce malware into the project. +- Does the explanation of the PR make logical sense and appear to match (as + near as you can tell) the changes in the code? Does it seem sensible, even + if you don't understand all the details? +- Does the test pass all of our existing CI tests? (If it does, at least we + are reasonably sure it doesn't break commonly used features.) +- Is there any evidence that the changes have been tested? Ideally, something + already in, or added to the testsuite by this PR, exercises the new or + altered code. (Though sometimes a patch addresses an edge case that's very + hard to reproduce in the testsuite, and you have to rely on the submitter's + word and the sensibility of their explanation.) +- Not required, but nice if we can get it: You're an expert in the part of + the code being changed, and you agree that this is the best way to achieve + an improvement. +- Any objections should be explained in detail and invite counter-arguments + from the author or other onlookers. A reviewer should never close a PR + unless it fails to conform to even the basic requirements of any submitted + PR, or if enough time has passed that it's clear that the PR has been + abandoned by its author. It's ok to ultimately say "no" to a PR that can't + be salvaged, but that should be the result of a dialogue. + +Obviously, you also want any review comments you give to be constructive and +helpful. If you don't understand something, ask for clarification. If you +think something is wrong, suggest a fix if you know how to fix it. If you +think something is missing, suggest what you think should be added. Follow the +expected kind and constructive tone of the project's community. + +### Submitter guidelines -- how to encourage good reviews and timely merges + +A few tips for submitters to help ensure that your PRs get reviewed and merged +in a timely manner and are respectful of the reviewers' time and effort: + +- Aim for small PRs that do a specific thing in a clear way. It is better to + implement a large change with a series of PRs that each take a small, + obvious step forward, than to have one huge PR that makes changes so + extensive that nobody quite knows how to evaluate it. +- Make sure your PR commit summary message clearly explains the why and how of + your changes. You want someone to read that and judge whether it's a + sensible approach, know what to look for in the code, and have their + examination of the code make them say "yes, I see, of course, great, just as + I expected!" +- Make sure your PR includes a test (if not covered by existing tests). +- Make sure your PR fully passes our CI tests. +- Make sure your PR modifies the documentation as necessary if it adds new + features, changes any public APIs, or alters behavior of existing features. +- If your PR alters the C++ API, make sure that the changes are also reflected + in the Python bindings and any other language bindings we support. +- If your PR adds to or alters any ImageBufAlgo function, make sure you have + exposed those changes with appropriate `oiiotool` commands as well. + +### Code review procedures -- in the ideal case + +In the best of circumstances, the code review process should be as follows for +**EVERY** pull request: + +- At least one reviewer critiques and eventually (possibly after changes are + made) approves the pull request. Reviewers, by definition, are not the same + person as the author. +- Reviewers should indicate their approval by clicking the "Approve" button in + GitHub, or by adding a comment that says "LGTM" (looks good to me). +- If possible, reviewers should have some familiarity with the code being + changed (though for a large code base, this is not always possible). +- At least a few work days should elapse between posting the PR and merging + it, even if an approving review is received immediately. This is to allow + additional reviewers or dissenting voices to get a chance to weigh in. +- If the reviewer has suggestions for improvement, it's the original author + who should make the changes and push them to the PR, or at the very least, + the author should okay any changes made by the reviewer before a merge + occurs (if at all practical). + +Not all patches need the highest level of scrutiny. Reviews can be cursory and +quick, and merges performed without additional delay, in some common low-risk +circumstances: + +- The patch fixes broken CI, taking the project from a verifiably broken state + to passing all of our CI builds and tests. +- The changes are only to documentation, comments, or other non-code files, + and so cannot break the build or introduce new bugs. +- The code changes are trivial and are obviously correct. (This is a judgment + call, but if the author or reviewer or both are among the project's senior + developers, they don't need to performatively pretend that they aren't sure + it's a good patch.) +- The changes are to *localized* and *low risk* code, that even if wrong, + would only have the potential to break individual non-critical features or + rarely-used code paths. + +Conversely, some patches are so important or risky that if possible, they +should be reviewed by multiple people, preferably from different stakeholder +institutions than the author, and ensure that the approval is made with a +detailed understanding of the issues. In these cases, it may also be worth +bringing up the issue up at a TSC meeting to ensure the attention of many +stakeholders. These include: + +- *New directions*: Addition of major new features, new strategic initiatives, + or changes to APIs that introduce new idioms or usage patterns should give + ample time for all stakeholders to provide feedback. In such cases, it may + be worth having the PR open for comments for weeks, not just days. +- *Risky changes*: Changes to core classes or code that could subtly break + many things if not done right, or introduce performance regressions to + critical cases. +- *Compatibility breaks*: changes that propose to break backward API + compatibility with existing code or data files, or that change the required + toolchain or depeendencies needed to build the project. +- *Security*: Changes that could introduce security vulnerabilities, or that + fix tricky security issues where the best fix is not obvious. + +### Code review compromises -- because things are rarely ideal + +We would love to have many senior reviewers constantly watching for PRs, doing +thorough reviews immediately, and following the above guidelines without +exception. Wouldn't that be great! But in reality, we have to compromise, +hurry, or cut corners, or nothing would ever get done. Some of these +compromises are understandable and acceptable if they aren't happening too +often. + +- Lack of reviews: If no reviews are forthcoming after a couple days, a "are + there any objections?" comment may be added to the PR, and if no objections + are raised within another few days, the PR may be merged by the author if + they are a senior developer on the project. This is a fact of life, but + should be minimized (with a rigor proportional to where the patch falls on + the "low risk / high risk" scale described above). If this is done, you + should leave an additional comment explaining why it was merged without + review and inviting people to submit post-merge comments if they + subsequently spot something that can be improved. +- Time-critical patches: urgent security fixes, broken CI or builds, critical + bugs affecting major stakeholders who are waiting for a repaired release, + fixes that are blocking other work or preventing other PRs from advancing. + In such cases, it is understandable for senior developers to try to speed up + the process of getting the changes integrated (though the very nature of + their criticality also indicates that it's worth getting a review if at all + possible). +- Failing tests: Sometimes a PR must be merged despite failing CI tests. + Usually this is for one of two reasons: (1) spurious CI failures are known + to be occurring at that time and are unrelated to the PR; (2) the PR is part + of a series of patches that are only expected to fully pass CI when the last + piece is completed. + +These changes are understandable, but we still are striving to reduce their +frequency. It is the responsibility of the senior developers, TSC members, and +major stakeholders to be monitoring the incoming PRs and helping with reviews +as much as possible, to ensure that review exceptions are rare. + +### Code review pathologies -- things to avoid + +These are anti-patterns that generally are indications of an unhealthy open +source project: + +* An author merging their own PR without any review comments that indicate + meaningful scrutiny or approval from another party, and without giving + adequate warning that a merge will occur if no reviews are received. +* PRs languishing for weeks or months without any review or merge. +* PRs that don't adequately test their changes (basically crossing your + fingers and hoping it works). +* PRs that are merged despite failing CI that might be related or, if + unrelated, that might be masking problems with the PR being evaluated. + +Again, sometimes these things happen out of necessity to keep the project +moving forward under constrained development resources. But we strive as much +as possible to ensure that they are rare exceptions.