My Pull Request Checklist

This is my personal checklist I have shared with teams to think about before submitting a Pull Request…

I have found people miss many things so a checklist is a good habit/practice to adopt to minimize back and forth cycles during a Pull Request.

General

• Get latest from source before creating a Pull Request — ONLY submit changes you have explicitly made in a Pull Request. Otherwise, you are at risk of over-writing changes you did not author.
• Does the code work? Does it perform its intended function, the logic is correct etc. per the PBI?
• Is all the code easily understood? (The goal is self documenting code — comments should be avoided in most cases. i.e. do not check in code comments // BAD)
• Does it have correct linting? (follow the README.MD for setting up prettier and running $ lint properly)
• Is there any redundant or duplicate code that can be refactored and reused? (i.e. remember your DRY, SOLID and KISS principles)
• Is the code as modular as possible? Can any of it be reused outside of given application? NPM Modules possibility’s?
• Can any logging or debugging code be removed?
• Are the observables been properly unsubscribed? (Angular…) Can async piping used instead of calling subscribe for a given observable?
• Does code follow prescribed architecture practices?
• Ensure PR passes CI server, re-running once or twice if it fails. Failure could be due to other issues, so determine whether the PR is responsible if failure occurs.
• Request changes to the code and/or additional unit tests if any issues are found.
• Are good names used for classes, enum, struct, methods, and variables?

Documentation or PR explanation

• Is any edge-case handling described?
• Is the use and function of third-party libraries documented and approved?
• Has the README.MD, CHANGELOG.MD or any other relevant docs / communication channels been updated?

Testing

• Is the code testable? (i.e. don’t add too many or hide dependencies, unable to initialize objects, test frameworks can use methods etc.)
• Do tests exist and are they comprehensive? (run $ npm test)
• Does code coverage reach %90 or greater for assigned work? (Include screenshots in PR with Code Coverage)
• Do unit tests actually test that the code is performing the intended functionality?
• Could any test code be replaced with the use of an existing API?
• Compare bundle size diffs

Lastly….

• IMPORTANT: ALL PULL REQUEST COMMENTS NEED A RESPONSE. We do not want pull requests merged prior to all comments being addressed, trivial or not. This is an essential part of the contract between reviewers and developers.
• DO NOT: Change code arbitrarily based on comments given — respond thoughtfully to the reviewers questions and provide reasoning behind decisions made.
• DO NOT: Assume the reviewer is always correct in their line of questioning or the suggestions they have made. Pull Requests are meant for dialogue and thus, a greater understanding with regard to intentions that can be achieved on both sides of a review. ** This will improve ability and quality throughout the code base over time.
• DO NOT: OPEN A NEW PULL REQUEST WITH PENDING CHANGES IN A PRIOR PULL REQUEST. We want to avoid at all costs losing comments by deprecating an in progress Pull Request in lieu of a new one where comments are then addressed. We lose transaction history and this defeats the purpose of our code quality safe-guard with pull requests.
• Pull Requests: Required Reviewers have 24 hours to do a first pass on a given pull request. Otherwise, a required reviewer is subject to be removed from their required reviewer obligation.