Code review and PR conventions - CDCgov/prime-simplereport GitHub Wiki

Code Review / PR Philosophy

SimpleReport's code review culture seeks to ensure we merge high quality code quickly. It's every developer's responsibility to balance the needs of quality and speed when working against timelines and product goals. To help with this, below are some recommended guidelines (not hard-and-fast rules). In all cases, use your discretion when making decisions!

1. Reviewers should approve a PR once it definitely improves the overall system, even if the PR isn’t perfect. Substance should always take precedence over style, understanding that style comments are welcome as long as they are denoted as nits. Reserve the blocking / requesting changes functionality when an important bug / security implication is introduced that definitely needs to be changed before merging.

2. Use code reviews to spread knowledge about the codebase, best practices, and other learnings around. As of this writing, the engineering team has decided that if something straddles the fence between a nit and a required change, we should err on the side of improving code quality rather than speed. In other words, reviewers should over-communicate in the comments! If something crossed your mind, sparked a question, or if you have a suggestion, leave it in the PR thread so the requester can learn. As a requester, take these comments graciously and give more context (ie "we have a parallel PR that will address this issue" or "this PR is getting too big / has been out too long so I'll address this in a followup") or fix them.

3. Code reviews should be done quickly so as to optimize the overall velocity of the development team, even if it comes at the cost of individual developer velocity. This optimization ensures the team is shipping high-quality software as fast as possible, even if individual developers move more slowly. The below list is how you should prioritize your time.

   1. Pages / on call escalations (if on call)
   2. Support escalations (if on call and to maintain the ~24 turnaround time)
   3. Code review
   4. Writing new code
   5. Dependabot PRs (maintain 1 week turnaround time)
   6. Personal discretion trumps everything

4. If a higher priority item comes in while you're in the middle of a focused task, the above list doesn't mean you should interrupt yourself (unless it's an urgent on-call page). Instead, finish what you're working on and prioritize the next task (eg after lunch, a meeting, or a break) according to the above ordering.

5. We have a daily Slack bot reminder to check PR reviews, and the team should try to submit PR reviews according to this cadence, if not faster. Keep in mind time zone differences when submitting reviews (ie it's ok for east coast colleagues to submit reviews the first thing in the morning if a west coast colleague puts it in at the end of their day.) Note this turnaround time refers to how long individual reviews come in, and not the entire process of moving a PR from open to merged. If you need to nudge folks to get more reviews because it is an urgent fix or the PR has been sitting around, do not hesitate to @here the internal simplereport dev Slack channel.

6. When submitting a PR, make sure all the automated checks are passing before requesting reviewers! It's awkward to request reviewers and have them give the green check, only to have a failing test, push a change, and require a new round of reviews. We strongly recommend using the draft PR functionality before requesting reviewers so that PR's run in CI at least once, just to make sure you didn't accidentally cause a test to fail that you weren't aware of. Reviewers have the right to not review PR's if any of the automated checks are failing.

7. In a code review, you should make sure that:

   • The code is well-designed / is clean.
   • The reviewer (you) has read and understands what's happening (if not, ask questions!).
   • Reasonable tests are written and well designed
   • The code has been run / smoke tested and it fixes the problem we're trying to solve / is on the ticket
   • The code follows existing style guidelines and best practices
   • Comments left are clear, useful, and mostly explain why instead of what.
   • If the code introduces new processes and/or are major architecture changes, that they're documented in the wiki or under the Architecture Decision Record page.

8. We try and stick with the 1 PR, 1 thesis rule in that each PR should accomplish one thing. If the PR is too big and unwieldy a reviewer might ask you to break it apart into smaller PRs. (Sometimes this is not feasible so please leave code comments to flag to reviewers areas you want special attention or to explain parts of the code).

Further reading /gathered resources are available below:

• How to make your code reviewer fall in love with you
• Conventional comments
• Google's Typescript and Java style guide

Git details

The convention for branch naming on this project is {firstName}/{ticketNumber}-{short-description}. This makes it easier for other developers to find your changes.

For most changes (e.g. feature work, dependency updates that affect how the app runs, bug fixes, etc...), it is strongly recommended that you smoke test them in a lower environment. This wiki page (Deploy) provides more information about how to deploy your changes.

For all changes, please ensure the PR checklist is completed before sending out for review. If you're making UI changes, make sure to screenshot and get approval from a designer or product manager, in addition to engineers.

We require two reviewers per changeset in the prime-simplereport repo and one reviewer in the prime-simplereport-site repo, and you cannot merge until all comments have been reviewed.