|
| 1 | +--- |
| 2 | +parent: Best Practices |
| 3 | +--- |
| 4 | + |
| 5 | +Code Review Best Practices |
| 6 | +========================== |
| 7 | + |
| 8 | +<!-- |
| 9 | +This was adapted from the OpenImageIO code review guide as a starting point. |
| 10 | +Obviously, some of it expresses preferences specific to that project and its |
| 11 | +leadership. But I think it provides enough good advice that it's a good |
| 12 | +starting point for an ASWF-wide best practices code review tips guide, and we |
| 13 | +can iterate from there to add good ideas from other projects, and to point out |
| 14 | +which sections may be highly subjective to each project. |
| 15 | +--> |
| 16 | + |
| 17 | + |
| 18 | +### Why do we code review? |
| 19 | + |
| 20 | +Code review is a process where someone other than the author of a patch |
| 21 | +examines the proposed changes and approves or critiques them. The main |
| 22 | +benefits are to: |
| 23 | + |
| 24 | +- Encourage submitters to ensure that their changes are well thought out, |
| 25 | + tested, and documented so that their intent and wisdom will be clear to |
| 26 | + others. |
| 27 | +- Directly find shortcomings in or suggest improvements to the proposed |
| 28 | + changes, and ensure that the changes are consistent with the project's best |
| 29 | + practices. |
| 30 | +- Minimize the amount of the code base that has only been seen or is only |
| 31 | + understood by one person. |
| 32 | +- Improve security and robustness of the code base by assuring that at least |
| 33 | + two people (submitter and reviewer) agree that every patch is reasonable and |
| 34 | + safe. |
| 35 | + |
| 36 | +### Reviewer guidelines -- what you should be looking for |
| 37 | + |
| 38 | +Code review is not difficult, and it doesn't require you to be a senior |
| 39 | +developer or a domain expert. Even if you don't particularly understand that |
| 40 | +region of the code and are not a domain expert in the feature being changed, |
| 41 | +you can still provide a helpful second pair of eyes to look for obvious red |
| 42 | +flags and give an adequate review: |
| 43 | + |
| 44 | +- This may seem too obvious to say explicitly, but a big part of review is |
| 45 | + just looking at the PR and seeing that it doesn't appear to deface, |
| 46 | + purposely/obviously break, or introduce malware into the project. |
| 47 | +- Does the explanation of the PR make logical sense and appear to match (as |
| 48 | + near as you can tell) the changes in the code? Does it seem sensible, even |
| 49 | + if you don't understand all the details? |
| 50 | +- Does the PR pass all of our existing CI tests? (If it does, at least we |
| 51 | + are reasonably sure it doesn't break commonly used features.) |
| 52 | +- Is there any evidence that the changes have been tested? Ideally, something |
| 53 | + already in, or added to the testsuite by this PR, exercises the new or |
| 54 | + altered code. (Though sometimes a patch addresses an edge case that's very |
| 55 | + hard to reproduce in the testsuite, and you have to rely on the submitter's |
| 56 | + word and the sensibility of their explanation.) |
| 57 | +- Not required, but nice if we can get it: You're an expert in the part of |
| 58 | + the code being changed, and you agree that this is the best way to achieve |
| 59 | + an improvement. |
| 60 | +- Any objections should be explained in detail and invite counter-arguments |
| 61 | + from the author or other onlookers. A reviewer should never close a PR |
| 62 | + unless it fails to conform to even the basic requirements of any submitted |
| 63 | + PR, or if enough time has passed that it's clear that the PR has been |
| 64 | + abandoned by its author. It's ok to ultimately say "no" to a PR that can't |
| 65 | + be salvaged, but that should be the result of a dialogue. |
| 66 | + |
| 67 | +Obviously, you also want any review comments you give to be constructive and |
| 68 | +helpful. If you don't understand something, ask for clarification. If you |
| 69 | +think something is wrong, suggest a fix if you know how to fix it. If you |
| 70 | +think something is missing, suggest what you think should be added. Follow the |
| 71 | +expected kind and constructive tone of the project's community. |
| 72 | + |
| 73 | +### Submitter guidelines -- how to encourage good reviews and timely merges |
| 74 | + |
| 75 | +A few tips for submitters to help ensure that your PRs get reviewed and merged |
| 76 | +in a timely manner and are respectful of the reviewers' time and effort: |
| 77 | + |
| 78 | +- Aim for small PRs that do a specific thing in a clear way. It is better to |
| 79 | + implement a large change with a series of PRs that each take a small, |
| 80 | + obvious step forward, than to have one huge PR that makes changes so |
| 81 | + extensive that nobody quite knows how to evaluate it. |
| 82 | +- Make sure your PR commit summary message clearly explains the why and how of |
| 83 | + your changes. You want someone to read that and judge whether it's a |
| 84 | + sensible approach, know what to look for in the code, and have their |
| 85 | + examination of the code make them say "yes, I see, of course, great, just as |
| 86 | + I expected!" |
| 87 | +- If you have extensive formatting changes or are moving existing code around, |
| 88 | + it is strongly suggested that those changes be submitted as a separate PR |
| 89 | + from anything that changes code logic. It is very hard to review code |
| 90 | + changes when code movement or reformatting obscure exactly what has changed |
| 91 | + and what has not. |
| 92 | +- Make sure your PR includes a test of the new or changed functionality (if |
| 93 | + not covered by existing tests). |
| 94 | +- Make sure your PR fully passes the project's CI tests. Be kind to reviewers' |
| 95 | + inboxes and attention by first pushing to your own fork and making all the |
| 96 | + fixups needed to pass CI *before* submitting the PR that will fail CI and |
| 97 | + leave reviewers unsure when it's complete enough to review. (You will need |
| 98 | + to ensure that your own fork has CI enabled.) |
| 99 | +- Make sure your PR modifies the documentation as necessary if it adds new |
| 100 | + features, changes any public APIs, or alters behavior of existing features. |
| 101 | +- If the project has multiple language bindings (e.g., a C++ library with a |
| 102 | + Python binding as well), make sure that any API functionality you have added |
| 103 | + is present (and tested) for all the language bindings so they don't get out |
| 104 | + of sync. |
| 105 | + |
| 106 | +### Code review procedures -- in the ideal case |
| 107 | + |
| 108 | +In the best of circumstances, the code review process should be as follows for |
| 109 | +**EVERY** pull request: |
| 110 | + |
| 111 | +> [!NOTE] |
| 112 | +> The following advice is generic, and we expect some projects to have |
| 113 | +> different specific requirements for the number and identity of reviewers, |
| 114 | +> how long a PR must be open to review, etc. |
| 115 | +
|
| 116 | +- At least one reviewer critiques and eventually (possibly after changes are |
| 117 | + made) approves the pull request. Reviewers, by definition, are not the same |
| 118 | + person as the author. |
| 119 | +- Reviewers should indicate their approval by clicking the "Approve" button in |
| 120 | + GitHub, or by adding a comment that says "LGTM" (looks good to me). |
| 121 | +- If possible, reviewers should have some familiarity with the code being |
| 122 | + changed (though for a large code base, this is not always possible). |
| 123 | +- At least a few work days should elapse between posting the PR and merging |
| 124 | + it, even if an approving review is received immediately. This is to allow |
| 125 | + additional reviewers or dissenting voices to get a chance to weigh in. |
| 126 | +- If the reviewer has suggestions for improvement, it's the original author |
| 127 | + who should make the changes and push them to the PR, or at the very least, |
| 128 | + the author should okay any changes made by the reviewer before a merge |
| 129 | + occurs (if at all practical). |
| 130 | +- It is permissible to request the "Copilot Review" from GitHub, but keep in |
| 131 | + mind that (a) it should augment, and not be treated as being a substitute |
| 132 | + for human developer reviews; (b) if any of its suggestions are incorporated |
| 133 | + into the PR, the human PR author takes full responsibility for its |
| 134 | + correctness and provenance, just as if they had written that code from |
| 135 | + scratch themselves. |
| 136 | + |
| 137 | +Not all patches need the highest level of scrutiny. At the discretion of the |
| 138 | +project leader, reviews can be cursory and quick, and merges performed without |
| 139 | +additional delay, in some common low-risk circumstances: |
| 140 | + |
| 141 | +- The patch fixes broken CI, taking the project from a verifiably broken state |
| 142 | + to passing all of the CI builds and tests. |
| 143 | +- The changes are only to documentation, comments, or other non-code files, |
| 144 | + and so cannot break the build or introduce new bugs. |
| 145 | +- The code changes are trivial and are obviously correct. (This is a judgment |
| 146 | + call, but if the author or reviewer or both are among the project's senior |
| 147 | + developers, they don't need to performatively pretend that they aren't sure |
| 148 | + it's a good patch.) |
| 149 | +- The changes are to sufficiently *localized* and *low risk* code, that even |
| 150 | + if wrong, would only have the potential to break individual non-critical |
| 151 | + features or rarely-used code paths. (Remember, this still assumes that CI |
| 152 | + fully passes and test coverage includes the feature being altered by the |
| 153 | + PR.) |
| 154 | + |
| 155 | +Conversely, some patches are so important or risky that if possible, they |
| 156 | +should be reviewed by multiple people, preferably from different stakeholder |
| 157 | +institutions than the author, and ensure that the approval is made with a |
| 158 | +detailed understanding of the issues. In these cases, it may also be worth |
| 159 | +bringing up the issue up at a TSC meeting to ensure the attention of many |
| 160 | +stakeholders. These include: |
| 161 | + |
| 162 | +- *New directions*: Addition of major new features, new strategic initiatives, |
| 163 | + or changes to APIs that introduce new idioms or usage patterns should give |
| 164 | + ample time for all stakeholders to provide feedback. In such cases, it may |
| 165 | + be worth having the PR open for comments for weeks, not just days. |
| 166 | +- *Risky changes*: Changes to core classes or code that could subtly break |
| 167 | + many things if not done right, or introduce performance regressions to |
| 168 | + critical cases. |
| 169 | +- *Compatibility breaks*: changes that propose to break backward API |
| 170 | + compatibility with existing code or data files, or that change the required |
| 171 | + toolchain or dependencies needed to build the project. |
| 172 | +- *Security*: Changes that could introduce security vulnerabilities, or that |
| 173 | + fix tricky security issues where the best fix is not obvious. |
| 174 | + |
| 175 | +### Code review compromises -- because things are rarely ideal |
| 176 | + |
| 177 | +We would love to have many senior reviewers constantly watching for PRs, doing |
| 178 | +thorough reviews immediately, and following the above guidelines without |
| 179 | +exception. Wouldn't that be great! But in reality, we have to compromise, |
| 180 | +hurry, or cut corners, or nothing would ever get done. Some of these |
| 181 | +compromises are understandable and acceptable if they aren't happening too |
| 182 | +often. |
| 183 | + |
| 184 | +- Lack of reviews: If no reviews are forthcoming after a few days, a "are |
| 185 | + there any objections?" comment may be added to the PR, and if no objections |
| 186 | + are raised within another few days, the PR may be merged by the author if |
| 187 | + they are a senior developer on the project. This is a fact of life, but |
| 188 | + should be minimized (with a rigor proportional to where the patch falls on |
| 189 | + the "low risk / high risk" scale described above). If this is done, you |
| 190 | + should leave an additional comment explaining why it was merged without |
| 191 | + review and inviting people to submit post-merge comments if they |
| 192 | + subsequently spot something that can be improved. |
| 193 | +- Time-critical patches: urgent security fixes, broken CI or builds, critical |
| 194 | + bugs affecting major stakeholders who are waiting for a repaired release, |
| 195 | + fixes that are blocking other work or preventing other PRs from advancing. |
| 196 | + In such cases, it is understandable for senior developers to try to speed up |
| 197 | + the process of getting the changes integrated (though the very nature of |
| 198 | + their criticality also indicates that it's worth getting a thorough review |
| 199 | + if at all possible). |
| 200 | +- Failing tests: Sometimes a PR must be merged despite failing CI tests. |
| 201 | + Usually this is for one of two reasons: (1) spurious CI failures are known |
| 202 | + to be occurring at that time and are unrelated to the PR; (2) the PR is part |
| 203 | + of a series of patches that are only expected to fully pass CI when the last |
| 204 | + piece is completed. |
| 205 | + |
| 206 | +These changes are understandable, but we still are striving to reduce their |
| 207 | +frequency. It is the responsibility of the senior developers, TSC members, and |
| 208 | +major stakeholders to be monitoring the incoming PRs and helping with reviews |
| 209 | +as much as possible, to ensure that review exceptions are rare. |
| 210 | + |
| 211 | +### Code review pathologies -- things to avoid |
| 212 | + |
| 213 | +These are anti-patterns that generally are indications of an unhealthy open |
| 214 | +source project: |
| 215 | + |
| 216 | +* An author merging their own PR without any review comments that indicate |
| 217 | + meaningful scrutiny or approval from another party, and without giving |
| 218 | + adequate warning that a merge will occur if no reviews are received. |
| 219 | +* PRs languishing for weeks or months without any review or merge. |
| 220 | +* PRs that don't adequately test their changes (basically crossing your |
| 221 | + fingers and hoping it works). |
| 222 | +* PRs that are merged despite failing CI that might be related or, if |
| 223 | + unrelated, that might be masking problems with the PR being evaluated. |
| 224 | + |
| 225 | +Again, sometimes these things happen out of necessity to keep the project |
| 226 | +moving forward under constrained development resources. But we strive as much |
| 227 | +as possible to ensure that they are rare exceptions. |
0 commit comments