Rules to Better Pull Requests - 15 Rules

Everybody strives to be perfect, but mistakes are normal, and it is easy for a developer to skim over errors when they've read their own code code hundreds of times!

Pull requests are the best way to ensure that two sets of eyes see every change - so the responsibility is never solely on the person writing the code.

When Pull Requests are enabled, developers must create a branch and make their changes on that branch. Then they submit a Pull Request to merge their changes back into main. Each pull request must be approved by another reviewer.

Useful resources - learn about Pull Requests

• Pull Requests in GitHub
• Pull Requests in Azure DevOps

Video: What is a Pull Request? (3 min)

As a software developer, you are going to create Pull Requests (PRs) that you want to be easy for others to review and approve. The quality of a Pull Request can vary - from cryptic to well-written.

Including a little bit of context helps your reviewer understand changes quickly so they can review your PR faster and give better suggestions.

There are 2 easy things you can do to improve your Pull Request:

1. Write a concise and self-explanatory title

The key to writing a concise Pull Request is to base the PR itself on a PBI / issue.

Good titles cover:

a. What the Pull Request will do

b. How the Pull Request achieved it

c. Emojis! Follow the GitMoji.dev standard

Examples:

PBI title: Product Backlog Item 100359: "Desktop App | Exporting occasionally failed"

Having the "What" information allows the reviewers to quickly understand what this is about while having the "How" can help the reviewer to quickly understand how your PR solved the problem. Sometimes we might want to put the "How" in the PR body if it is too long or hard to explain in one sentence.

2. Write a clear and concise description

The Pull Request description is a medium for the developer to tell the reviewers what the changes are about.

Good descriptions cover:

a. Context:

• Relates to #{{ ISSUE NUMBER or URL}} (⚠️ see avoid linking any Issues that you do not want to close)
• From email: {{ SUBJECT }} (like the rule Warn then call)
• As per my conversation with {{ NAME }} (like the rule Do you send as per our conversation emails)
• I noticed a problem: {{ DESCRIBE }}

b. Did you do pair or mob programming?

• Do you use Co-Creation Patterns?
• Example: Worked with @bob, @mary and @jane

c. What the PR is about and why you raised it

d. How the PR will achieve the feature / fix the bug / other goals

e. Include a screenshot/GIF/Done Video to help the reviewer to understand the changes (e.g. styling changes)

f. Tell he reviewers if there is an area you are uncertain about, e.g.: I'm looking for feedback on this code

Try to make generic comments that objectively summarize your changes. This way the reviewer will know what to expect and confirm the changes by looking at the file diffs.

FAQs

Q: Are you making many small changes?

A: You should summarize by saying: “Improved readability” OR “Fixed typos and grammar”.

Q: Are the changes big and complex?

A: You should include a demonstration of the change.
E.g. A screenshot to show text/UI changes, or a Done video to demo functionality changes.

Q: Are you using a GUI editor (like Netlify or Tina)

A: If you're using a GUI editor for your PRs, you may need to go to the PR afterward and update the description to include the context

An "over-the-shoulder" review is one of the best ways to avoid merge debt. When a pull request (PR) is ready to be reviewed, get someone with you either in-person or on call, and go through the PR together. This not only allows you to demo the content of the PR but also talk with the person taking feedback when needed.

When you have finished coding, don't just create a PR and throw it over the fence. Part of finishing a PR is getting it approved.

The best way to get it approved is via an "over the shoulder" review
- Adam Cogan

Drafting PRs

A good way to avoid someone merging your PR before you have done an over the shoulder review is to keep the Pull Request in draft mode until you are ready for it to be reviewed for merging.

If you are in a team (i.e. an internal contributor), it is the PR author's responsibility to get a PR reviewed and action feedback ASAP.
As an internal contributor it's a good idea to have a call with the PR reviewers.

If you are not part of the team (i.e. an outside contributor), reviewing the PR is the responsibility of the repo maintainers. Actioning the feedback is still the responsibility of the PR author. As an outside contributor, it's a good idea to chase the reviewers by reaching out with a comment on GitHub, or through the repo's community (e.g. Discord channels).

When a PR is created but left open for a long time, it becomes stale. Stale PRs equate to merge debt because the longer the PR stays open, the more changes occur on the main branch and the harder it is to merge back in.

Merge debt refers to the amount of work a PR has to undergo before it can be merged into the main branch. If the PR is brand new, the amount of work required to merge is near to none, but as the PR stays open, the more work gets done on the main branch, leading to more work needing to be done to ensure the PR is up-to-date and able to be merged.

Merge debt can be avoided by:

• Ensuring PRs don't stay open for too long
• Conducting daily reviews on repos to ensure all PRs that can be merged are merged
• Ensuring that once a PR is ready to be merged, an "over-the-shoulder" review occurs.

Pull Request templates are a great way to communicate expectations to developers. You should can create different PR templates for different types of PRs. For example, you can have a PR template for bug fixes, a PR template for new features, and a PR template for refactoring. You are also able to create specific PR templates for specific code paths.

You can read more about PR templates in the GitHub docs at Creating a pull request template for your repository

When creating a PR template, think of how you can help developers create great PRs

Tip: You can use comments in the markdown as above. These comments will not show when the PR is created, and is only visible when editing the description.

For a great Pull Request template, take a look at @SSWConsulting/SSW.GitHub.Template

For the original article check out Don't "Push" Your Pull Requestsby Ilya Grigorik.

Open source software projects love it when the community get involved and pitch in.

It's great when someone fixes a bug.

A short pull request to fix a small problem is easy to review and accept.

It's great when someone adds a much-needed feature...as long as the feature fits in with the project the core contributors have for the project...and it meets the existing coding patterns and engineering standards.

This is where frustration often starts to set in on open source projects.

A contributor has a great idea for a feature, or identifies an area where they can add value and does so without consulting with the core contributors who have often spent hundreds of hours working on the project.

Their contribution might solve their problem, but after it has been accepted it will most likely be left for the core contributors to maintain.

Pull requests are a fundamental feature of collaborative software development, allowing contributors to propose changes to a project and receive feedback from other developers. When reviewing a pull request, it can be tempting to make additional changes beyond those requested by the PR creator.

Certain projects (E.g. SSW.Rules) allow these additional edits, without the need for extra reviews by someone else. Knowing that, it's crucial to make sure the changes are correct, necessary and beneficial to the project before adding them.

Most common scenarios where editing an existing PR is encouraged:

• Fixing typos
• Grammar improvements
• Formatting (Markdown) fixes/improvements

For the cases where your wanted change can serve as a learning opportunity to others, it is always best if you ask them to action by requesting a change. This way the extra change can be avoided in the future.

Ultimately, the decision to add additional changes to a pull request should be made based on the needs of the project and the expertise of the contributors involved. It's important to carefully consider the impact of any additional changes and ensure that they are aligned with the project standards.

Assume you are creating a cool new feature. First you will create a new branch, create some commits, check it works, and submit a pull request. However, if you are not happy with the feature then don’t just delete the branch as normal. Instead, create a pull request anyway and set the status to Abandoned. Now, you can continue to delete your branch as normal.

This makes sure that we have a historical log of work completed, and still keeps a clean repository.

When the Product Owner verbally requests a change to a PBI, how do you update the PBI to reflect the change and also keep track of the conversation?

You could send yourself a "To Myself" email and update the PBI description accordingly, but only those people included in the email chain are aware of the conversation. Only send a "To Myself" email when there is no Product Backlog that is related to the request, otherwise you should create or update a PBI and @ mention the Product Owner and other relevant people (@ mentioned people will still receive an email).

Instead, what you should do is use the discussions feature in the PBI and mention the user using "@<username>".The benefits of using comments are:

• Quick and easy, no need to compose an email
• History is visible to anyone looking at the PBI (with email, if you don’t cc them, they wouldn’t have a clue)
• Easy to see all important notes/comments in one place instead of digging through email

When someone (especially the PO) asks you to fix a PBI, mention that person in the PBI comments so they know when it’s fixed.

Example: When replying to "Hey XXX, can you please fix PBI 123?"

Azure DevOps PBIs

To create a new PBI in your Azure DevOps project:

1. Navigate to Boards | + New Work Item and select the type that best suits your item
2. Enter your PBI title
3. @ mention your desired user in the description

It is also good practise to use @ mention in the discussion to track changes and request test pleases. Try formatting your mentions like an email to clarify both accountability and responsiblity and identify the current status of the project.

GitHub Issues

Related Links

• Interruptions - Do you know that IM interrupts?
• Suggestion to Microsoft Azure DevOps - Help me know there is an image in the Work Item
• Zendesk - Do you reply to the correct Zendesk email?

GitHub provides a way to link issues to PRs. This is useful to see what PRs are associated with what issues. However, when you make this link, the issue will close when the PR is merged.

This is not a good idea because it can cause issues to be closed prematurely. This can lead to confusion and lost work.

Issues should not be closed until all the tasks are complete and have a done comment as per Do you close PBIs with context?

Tip: Avoid using closing keywords e.g. "closes #123" or "fixes #123" in PR descriptions. This will automatically link the issue to the PR and close it on PR merge. Instead, use terms like "relates to #123" or "addresses #123" to link the issue to the PR without closing it.

This was a feature GitHub added but it is not a good idea to use it, if you agree the behaviour should be changed, go upvote this discussion https://github.com/orgs/community/discussions/17308

When you are working on an open source project, you will often get pull requests from contributors. When you merge these pull requests, you should use the squash and merge option. This will squash all the commits into one commit and then merge it into the target branch. This is a good practice because it keeps the commit history clean and easy to read. It also makes it easier for other developers to understand what changes were made in each pull request.

Commit Message

Like any commit message, the PR commit message should be short and descriptive. The easiest way to achieve this is to combine the PR title and commit details.

In order to get GitHub to use your commit details by default you need to change the configuration for the repository.

Limit merge types

You should limit the merge types that are allowed on your repository. This makes it easy for everyone to know the expected merge type of the repository when the PR is merged.

Automatically delete head branches

After a branch is merged into the target branch, you'd not want to continue development on the previous branch as the changes were squashed in. It's always a good idea to create a new branch after a PR is merged from the target branch if you have additional changes. To make this easier, you can configure GitHub to automatically delete the branch after the PR is merged.

Other configuration

If you find that pull requests are often approved but for some reason not merged in, you may want to enable auto-merge. This will automatically merge the PR when all the required checks have passed.

You may want to enable Always suggest updating pull request branches, this can be done from Repo Settings | Pull Requests, this provides contributors with an easy way to update their branch from the target branch without performing the merge themselves on their local machine.

Getting started with a new repository can be daunting, especially if you are new to the project. Having a standard set of pull request workflows can help you get started and make sure you are following the same process as everyone else on the team.

A few standard workflows helps developers see a consistent process across all repositories. This makes it easier for developers to get started with a new repository and makes it easier for developers to move between repositories as the feedback they get from each pull request is consistent.

Below are a few standard workflows that you can use in your repositories.

PR - t-shirt size the pr

This workflow uses the microsoft/PR-Metrics action to update each pull request with information that helps ensure engineers keep PRs to an appropriate size with appropriate test coverage, while informing reviewers of the expected time commitment for a thorough review of the code.

You can find the workflow at SSWConsulting/SSW.GitHub.Template/.github/workflows/pr-metrics.yml

PR - Manage Stale PRs

This workflow creates adds labels to pull requests as they age.

The workflow will also ping the author of the pull request after around 36 hours and remind them about merge debt

You can find the workflow at SSWConsulting/SSW.GitHub.Template/.github/workflows/pr-manage-stale.yml

As a repository maintainer you would generally setup a branch policy which may include a certain number of reviewers before a pull request can be completed, this could also include that certain reviewers are required instead of this being wide open to just anybody. This is a great way to ensure that code is reviewed before it is merged into the main branch.

However, this does not mean that you should not review pull requests that you are not required to review.

Let's take a look at some of the benefits of reviewing pull requests, even if you are not a required reviewer.

As a junior developer, you may not feel comfortable reviewing a senior developer's code, but you should. You may not be able to provide any feedback around standards and best practices, but you can ask questions. This is a great way to learn and understand why the senior developer did something a certain way.

When you ask questions in a pull request, you are giving the author the opportunity to take a step back and think about what they have done and how they've done it, explain their thought process and potentially where you could learn more about the approach. Alternatively, you could point out a different way of achieving the same result which could be a better approach and they could change their PR.

• ✅ You could learn something new
• ✅ The author could learn something new
• ✅ Congrats... you've just created some micro-documentation that may never have existed before

The more people have approved a pull request, the more confidence you can have that more people in the team have looked at and agree that the code should be merged into the branch. This is effectively the same approach as getting a "Checked by xxx" on an email, see Do you use 'Checked by xxx'?.

• ✅ Shows the team you understand and agree with the code changes

These days Pull Requests are the de facto standard for getting code reviewed. Once a developer has finished their change, they will typically submit a Pull Request and move on to their next task. This allows for an asynchronous process to take place which may seem like a good idea, but often is not and can also lead to inefficiencies.

❌ Problem - Inefficient Code Reviews

Inefficient code reviews can be caused by:

• Requesting feedback too late
• Receiving feedback too slow
• Creating large Pull Requests
• Excessive context switching
• Too much work in progress
• Unclear feedback

✅ How to Make Code Reviews More Efficient

• Author - Do over the shoulder reviews
• Author - Ask for feedback early before the PR, if you are uncertain that you're on the correct path

• Limit work in progress

  • Author - Make sure your Pull Requests are merged, before starting a new task
  • Reviewer - Prioritize Pull Requests before starting a new task

• Author - Create small Pull Requests

  • This requires a smaller block of time to review which makes it easier for the reviewer to find the time
  • Less risk - reduces the chance of an incorrect approach being taken
  • Get quality feedback - small blocks of code are easier to digest

• Reviewer - When reviewing asynchronously

  • Be explicit and suggest the exact code changes where possible (GitHub has a feature for this, see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request)
  • Call the developer for more complicated changes

The Ultimate Solution - Co-Creation Patterns

Small Pull Requests have many benefits as outlined above. However, each Pull Request comes with an overhead and making Pull Requests too small can introduce unnecessary waste and negatively affect the throughput of code. In order to not lose throughput with small PRs, reviewers need to react fasterThat leads us to synchronous, continuous code reviews and co-creation patterns

So, with the async way of working, we’re forced to make a trade-off between losing quality (big PRs) and losing throughput (small PRs).

We can avoid this by using co-creation patterns. As a general rule, Pull Requests with less than 20 lines of code, and larger changes with a degree of complexity/risk, make good candidates for co-creation.

The idea is that you do small PR's but also limit WIP. If you create several small PR's quickly and are waiting for code reviews, you can become blocked very quickly. By co-creating, the small PR's get reviewed & merged instantly which avoids getting blocked and enables you to smash out loads of small PRs! 💪

Daniel Mackay - SSW Solution Architect

Patterns

Co-creation patterns can take several forms:

1. Pair-programming: starting, reviewing and finishing a change together
2. Mob-programming: working in a small group, that collectively has all skills required

Advantages of co-creation

Co-creation allows us to have both quality and throughput by providing the following advantages:

1. More context when reviewing
2. Higher quality
3. Faster communication
4. Faster course correction
5. Less delay - no waiting
6. Eliminates context switching - working on a change together reduces WIP which further increases throughput
7. Emotions are removed - instead of having an 'author' and 'critic', the code is created together.

Managing webpages can be challenging, especially in projects with many contributors. When editing a page, a common problem is not knowing who the original author was. This is bad, because it's important not to change something that was there for a reason!

The best way to solve this is by having a page owner for each webpage.

Why?

A designated page owner ensures someone is responsible for the accuracy of the page content. It avoids confusion about who to approach for major changes, and it allows the page owner to keep an eye on changes.

Steps

1. Markdown files - Add an 'owner' field in the frontmatter metadata for each page.
2. New pages - The field should be required when creating a page.
3. Pull requests - Automatically add the page owner as a reviewer for any pull requests that modify their page.

Tip: this can be done automatically with a GitHub Action (or similar automation)

✅ The "owner" (person responsible) is aware of and can approve any changes

✅ People know who to consult about changes to the page

🤔 We don't use CODEOWNERS for this because we don't want to block pull requests for minor edits

❌ When trying to work out who is the page owner, version history is not good enough - often the creator of a page is not the actual author e.g. a dev makes the page for a Marketing person