Parsons Best Practices for Pull Request Review

This guide covers most of what you would need to know to add reviews to Parsons PRs. It does not assume any previous experience contributing to open source software or reviewing PRs. Please let us know if you spot mistakes or have questions!

There is also a recorded training which covers much of this guide.

First Things First: Join the Community

Please come join us in the Parsons Slack!

First, we really want to meet you. The Parsons community is just as focused on building skills and relationships in the community as it is on building Parsons itself. We would genuinely love to get to know you.

Second, you will probably get stuck somewhere in the process of contributing. While you can also ask for help with a specific issue by posting in the issue tracker, a quicker and easier way to get advice is to post to the #getting-help or #contrib channel in Slack.

To join the slack, email [email protected].

What are Pull Request Reviews?

Pull Request Reviews are a chance for the community to provide feedback to other community members’ code contributions to Parsons before they are merged to main. This is also a great way for beginners as well to ask questions about code they are unfamiliar with and learn from their fellow community members!

Draft Pull Requests

Draft pull requests are a means for contributors to show progress on code that they’re in the middle of writing. Generally, these don’t need review until the contributor specifically asks for it. However, if there hasn’t been any updates in a while, you can ping the contributor by adding a comment to check-in on their progress.

Default Pull Request Workflow

For regular pull requests, here are some pointers for what to write in your reviews:

First Steps

1. Make sure that the PR has a descriptive header. If its meaning is unclear to you or if you have any further questions, please follow-up with the contributor.

2. Secondly, check the automated tests at the bottom of the PR:

   

3. Even if the docs-build tests pass, for significant docs changes (beyond a small addition or typo fix) it’s best to check manually. A guide to building the docs locally can be found here.

4. When checking unit tests, if there are issues with the first any steps other than “install dependencies”, “run tests” or “check linting”, this is a problem with the test suite itself and the maintainer team needs to tackle it. You can notify Shauna or the TMC engineering team in the Slack maintainers channel.
   

   

5. If “install dependencies”, “run tests” or “check linting” fails, you can notify the PR requester that they need to fix these issues before the code can actually be reviewed. Make sure to let them know that if they’re having any trouble, they can ask for help. If it is a linting issue, you can link our linting guide on how to run black.

6. If the code change is an infrastructure change (i.e. at the top level of the repo) or the change will impact how tests are run, you would need a review from Shauna or a member of TMC engineering, so, feel free to ping them in the comments!

Starting the Review

Once all the tests have passed, you can start writing the review!

PRs need at least one review from someone with approval power, but we encourage all members to submit reviews as a learning opportunity. Feel free to qualify suggestions with “I’m not sure” if you don’t know whether something ought to be changed. Also: submitting questions on sections of code you don’t understand is really helpful! If you have questions, it may be a sign that the code is not written as cleanly or documented as clearly as possible.

To leave comments on the code directly, go to the “Files changed” section and skim through the code to start providing feedback.

You can either leave individual comments, or do a full review. To leave individual comments, hover over the line of code you’d like to comment on and click the little blue button that appears. Leave your comment in the comment box and select the “add single comment” option.

To start a full review, choose “start a review” in the comment box, or you can begin a review by choosing the green “Review changes” button.

Besides approving a PR, you can also add a comment or request changes. Changing the status like so would also help us keep track of what PRs need review for organizational reasons as well!

Possible PR Review Comments:

1. Make sure that the code follows Parsons coding conventions

2. Is the code as readable as possible? If you don’t understand something, just ask! Asking could be very useful in revealing unreadable code for others as well. Other questions to help determine if code is readable: Are global constants at the top of the file? Are local variables in lowercase and snake case? Are variables written in expressive names?

3. Some guidelines on logging as well:

   1. Does the code have enough logs to debug in production?

   2. Does the code have too many logs to make production noisy?

   3. Do all logs have useful prefixes?

4. Some coding structure best practices:

   1. Reusability and Modularity

      • Does the code repeat logic? Could it be moved into a reusable function?

   2. Separation of concerns

      • Are the different objectives of the script separated cleanly? For example, how something is loaded vs. transformed

   3. Single Responsibility Principle

      • Does each function do one thing and one thing only?

   4. Limiting complexity

      • How many nested conditional blocks are there? Could they be reduced?

5. Some pointers on documentation:

   1. Make sure that the documentation follows these guidelines

   2. If there are significant documentation changes (as in pages long), make sure to check them manually

   3. Also double check that the new addition shows up in the Parsons documentation sidebar and body. Usually, if the addition does not show up in the sidebar, the issue is usually missing code at the bottom of the page.

6. Some pointers on unit tests:

   1. Make sure new connectors and methods have unit tests! 

   2. Check they followed standard process of mocks to handle third party data (and not alternative methods)

Finally, here is a sandbox access guide to double check that the requester actually has access to it. However, if it’s difficult, the changes are minor, and the PR is submitted by someone well-trusted in the community or someone else has already reviewed this, you can skip this step. Although, if it’s a big change or the requester is a brand new person, it’s worth it to make the extra effort!

Last Steps Before Merging

1. Make sure to not bypass any of the steps at the bottom of the page before merging a PR (i.e. always make sure to update a branch before merging!)

2. For resolving merge conflicts, it’s a judgment call as to whether you, the requester, or the reviewer should fix them. To notify the requester, you can send them a message notifying them that they can fix it through the UI and ask if they require any help.

3. Also, make good use of the labels (especially when shouting out new community members and contributors). 

4. It’s also a good call to encourage as many reviewers as possible!

Final Notes

The PR review process is mostly a judgment call and we encourage you to make a guess and/or ask for help. The Slack #contrib channel is always there for you if you have any questions or have an extended conversation about PR reviews.