Skip to content

Instantly share code, notes, and snippets.

@manjufy

manjufy/reason-for-pr.md

Last active January 4, 2020 04:44
Show Gist options
  • Save manjufy/a91e5813e77eaf632f168ac6576c94da to your computer and use it in GitHub Desktop.
Save manjufy/a91e5813e77eaf632f168ac6576c94da to your computer and use it in GitHub Desktop.
Download ZIP
Raw
reason-for-pr.md

Reason for Pull Request

Quality Assurance?

  • Did we implement test, if so, did it pass?
  • Did the change add/remove tests? why?
  • Did the commit decrease quality measurably?
  • Did the commit introduce possible future technical debt? is it acceptable? → Its always good to get teams opinion and come to conclusion
  • Did any of the tooling indicate a decrease in maintainability or compliance?

In terms of Operation

  • Does the change satisfy the statement of work submitted for the change?
  • Does the change logic look reasonable?
  • Can I follow the logic change mentally, and not get lost

Customer perspective

  • From the end/admin/manager/etc users perspective does the change function as expected?

Team/Dev Perspective

  • The Why: Understanding the reason for the changes. Was this a bug? or a feature?
  • Logic: Dig into the logic to ensure that it all works like it should
  • Performance: Are there any unnecessary MySQL queries? Are indexes in place where they should be? how will this code perform when executed a million times? How will this affect our database.
  • Edge Cases: Since we are going to deploy the code lot of often than we used to do traditionally, its always good to think/deal with edge cases. Code works 99% of the time, but if given this scenario, it breaks. Then we can take a step back and look at the bigger picture of the change. How will this interact with other moving pieces of the code.
  • Code style and cleanliness: Are there bits that could be refactored to be more readable or more reusable? Are any . variables not quite clear based on their naming.
  • Questions: When not unfamiliar with change, ask questions. In my opinion it is the most valuable things when we ask questions. By explaining the changes to other team members, half of the time we would end up finding the bugs within our code. Secondly, we become more familiar with our code.
    • By asking questions, giving the author a chance to explain why they choose the path they took
    • As a reviewer will learn from the authors explanation
    • Asking questions prompt discussions. Discussion leads to technical debates, and eventually might lead to finding a better solution or a whole new idea
    • Asking questions one can learn a lot about the system, edge cases, cases we might not have thought about
    • ask don't tell. "The wise man does not give us the right answers, he poses the right questions"
    • As a developer, you get lot more clarity on your implementation when people asking questions. Sometimes a better understanding and sometimes better solution. Its a win-win
  • Awareness → Letting other team members to know about the change.
  • Sanity checking the codes for syntax errors, add/remove lines of code by accident, semantic errors, typos etc?
  • Any residual debugging statements?
  • Anything that feels like a mistake?
  • Find good code sections / good tests then publicly praise / thank the author.
  • As a reviewer if not confident about the change.
  • Everyone should be able to participate in the code review process as it helps the team to learn from each other.

Yes, there are lot of things to consider, and yes it may slow down the merge process a bit. But in my experience taking a little longer to review code can help prevent issues from getting to production and this is a step closer to achieving CI/CD automate process with confidence.

Rules of PR

  • What has changed between his feature branch and the dev branch
  • Checklist of Prerequisites (added documentation, added tests, ready to merge label etc)
  • How to use the feature
  • How the design has changed
  • Additional notes
  • Smaller the PR is better. Velocity of the PR getting merged quickly is very high.
  • Smaller the PR, easier to understand.
  • If we are implementing a new feature, sometimes PR can be understandably big. It would be good to wrap them around toggles/foggles.
  • When we open a PR it would be good to provide information about the PR.
    • Overview of changes
    • Link to a ticket
  • As soon as PR is opened, would be good if we can address it asap
  • To not to hinders the productivity of the developer, its good to address the PR asap. If no one is avail, its good to sit down with other developer to go through the changes and get approval and merge it
  • Comments can be very abstract. Would be good to leave comments with clear explanation and with example or just walk to the developer and explain

https://medium.com/osedea/the-perfect-code-review-process-845e6ba5c31

've learned a ton asking questions in PR (ex: "oh, so you can define a nested class in this language?", etc), but I've also used it with great effect to guide a more junior dev to realize ways they could improve their code and understand the "why" of one approach to solving a problem being better than another (ex: "so if we had to change this value here, what else would have to change?" or "if this value was null here, what would happen in the subsequent lines that follow?", etc) Ie "ask don't tell" can be an effective review technique for avoiding putting the author on the defensive.

References:

Reference: Medium and Dev.to

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment