Skip to content

Reviewing a Pull Request

Jump to bottom
Earl Duque edited this page Sep 30, 2023 · 3 revisions

Tips will be presented in blockquotes

What a good Pull Request will generally look like

  1. Open a Pull Request
  2. Under Assignees click on assign yourself

image

  1. Read the PR's title and description

Is it too vague or fails to describe what is being changed? See Failure Points below.

Does the description say that it's resolving an issue? See the below section on auto-resolving issues.

  1. Review the Files changed tab

image

  1. Verify the edits. On the left you will see all files added, edited, or deleted. Make sure to look through each one.

    • Do they follow that project's contributing guidelines?
    • Are the changes not frivolous/spammy?
    • Do the changes match the PR's title and description?
    • Do all changes belong in this Pull Request?

If not, see failure points below

  1. If everything looks good, click Review changes, Approve, (optional) leave an affirming comment like Looks good!. and then click Submit review

image

  1. The Conversation tab will automatically load. Verify that all checks have passed and that this Pull Request's branch has no conflicts with the base branch.

image

  1. Choose Squash and merge and then Confirm squash and merge

image

Auto-resolving issues

A pull request's description can say something like resolves #42 and upon the confirming a merge, the stated issue will automatically close.

Make sure that the issue they are claiming will be resolved will indeed be resolved by this Pull Request

The following keywords may be used:

  • close
  • closes
  • closed
  • fix
  • fixes
  • fixed
  • resolve
  • resolves
  • resolved

Did they use the wrong keyword like Solves or didn't use the keyword properly like Resolves issue #42 (instead of just Resolves #42) You can actually edit their description for them to fix the keyword. Do this before you hit merge.

Failure points

The Pull Request's title and description are too vague

Or does not describe what the contribution does.

  1. On the Conversation tab, leave a comment to inform them that Pull Requests are required to be informative (see our templates wiki page) and that they can either reopen the current pull request or submit a new one.
  2. Click Close pull request

image

The Pull Request does not follow that project's contributing guidelines

  1. On the Conversation tab, leave a comment to inform them that Pull Requests must follow the contributing guidelines in order to be approved and then inform them, to the best of your ability, of how it does not follow the guidelines. Use the relevant template to start your comment.
  2. Click Close pull request

The Pull Request's file changes appear to be incorrect or invalid

  1. On the Files changed tab, leave a comment on what is incorrect or invalid:
    • If a specific line is incorrect (or the first line of a block of incorrect lines), hover over the line and click on the blue + icon. Write your comment and click Start a review.
    • Add comments on other lines for other unique problems.

image

  1. Click on Finish your review (if you had line comments) or Review changes (if your comments are in general and not pertaining to specific lines)
  2. Use the relevant template and add any other comments regarding what needs changing
  3. Select Request changes and then Submit Review

image

  1. The Conversation tab should automatically load. Click Close pull request

image

The Pull Request has changes that do not belong in this pull request

  • Branches are confusing and some users do not realize they need a new branch for each pull request (if they are submitting them rapidly before their older pull requests can be approved). Use the relevant template and encourage them to chat with someone if they are still confused.
  • Some users will forget they playfully edited some files or tested some edits before getting to work.
  • ServiceNow apps that are imported into instances are notorious for sometimes mass editing files by just a little bit, resulting in a mass edit several files unintentionally. In most cases, we want them to go back and make sure their Pull Requests are clean. In some cases, talk to the project owner or other reviews on their opinion.
  1. On the Conversation tab, use a relevant template to let them know what is wrong
  2. Click Close pull request

image

The Pull Request is frivolous/spammy

Oh hell naw. Unless the person is known in our community and you decide to give them the benefit of the doubt, we should issue no warnings and take action against these users:

  1. Assign the spam label to the pull request
  2. Copy and paste the spam template and close the pull request

If the spam label does not exist for your repo, you can create it

Clone this wiki locally