Code review and PR conventions

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 guidelines the team as of 8/8/22 have drafted around code review. These are meant to be 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 overcommunicate 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. 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.

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.
   • 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.

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.

If you're merging large or complex changes, it is strongly recommended that you smoke test them in dev, test, or pentest. This wiki page (Deploy) provides more information about how to deploy your changes.

These three environments are roughly the same, with small configuration changes between each. This wiki page (Cloud Environments) provides more information about the different environments,

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, and you cannot merge until all comments have been reviewed.