Pull Requests.md

As we are on-boarding more developers on js-waku, I wanted to share some thoughts around Pull Request management.

js-waku's branching strategy

js-waku uses a GitHub Flow strategy:

• 1 long running branch: master from which releases happen and is always ready (do not break master)
• Developers create a branch from master to isolate their work, the branch name must be descriptive (e.g. feat/name-of-feature).
• A Pull Request is open to merge the developer's branch to master.

Pull Request Objectives

The primary objective of a Pull Request (in its literal sense) is to request for a series of patches to be pulled in master. The reasons to pull patches, ie, introduce a code change can be:

• For the user: Fix a bug, introduce or modify a feature/documentation so it can later be released and available to users.
• For the maintainer(s): Refactor to make a specific piece of code easier to maintain or extend, stabilize CI, add non-regression tests.

Naturally, a PR should aim to not do any of the opposite (introduce bugs, feature regression, destabilize C). Nor should it do neither of those (e.g. an aimless refactor or a feature worthless to users).

PRs that are not to be merged against master

The goal definition above states that all PRs are to be merged against master. There are few reasons why a maintainer may decide to open a PR that breaks this statement:

1. The PR is not to be merged: This PR can be open to demonstrate a code change or a PoC. The PR in itself is a documentation for other or future developers and will be closed without being merged.
2. The PR is not for master: The PR is opened to be merged against a different branch, against which a PR may already exist.
3. The PR is not ready to be merged: The PR either depends on other PRs to be merged first or it has been published to get some direction/help from other team members.

Some guidelines for js-waku:

1. If a PR is never to be merged, then it MUST remain as draft until closed.
2. This use case should not exist as what matters is what is merged in master, to be discussed more in the next sections.
3. In this case, it is best to keep the PR as draft until it is considered as ready to merge by the author.

Pull Request Optimization

The goal of a GitHub Pull Request is to ensure that the objectives defined above are reached:

• There is a consensus from other maintainers that this PR is either useful to the user or the maintainers.
• No regression or bug has been introduced, thanks to the non-regression tests run during the CI and review from other maintainers.

As a PR is meant to pull changes in master, then to optimize a PR is to shorten the time between publication and merging.

There are two steps to get a PR merged:

1. CI needs to pass
2. Reviewers need to approve

To ensure the CI passes, one need to not break any test. More generally, maintainers need to ensure the CI can run fast and remain stable.

We'll address below how to get approval from reviewers, ie, optimize for review.

Optimize for Review

Here are practices I encourage to adopt to get PR approved in a timely fashion.

PR not against master

As mentioned in the previous sections, a PR is meant to introduce changes in master, useful to the users or maintainers.

A PR that is not open against master but against another branch does neither of the goals above, it just brings changes into another branch.

Asking other maintainers to review a not master PR is a distraction:

• It does not reflect the code that will ultimately be merged against master
• It may serve as a double review, as someone will have to review the code (again) before it gets merged against master.

Hence, it is recommended to only ask for reviews of PRs against master.

If a maintainer prefers to open a not master PR as part of their workflow, it is their choice. However, they should be considerate of the time of other reviewers and avoid asking reviews.

PR Author Mandate

To optimize for a fast approval, the author should ensure the PR has the following properties:

The code changes are quick to read

• Total code changes under 400 lines
• Or, commits can be reviewed individually
• Even better: both

The benefit is obvious

There is a clear benefit for the user or maintainer. PR description and/or commit descriptions should be used to highlight the benefit.

The changes are coherent as a set

It is easier to review if the reviewer only has to think about one specific issue/concern/feature.

Refactoring, formatting and other minor issues being fixed at the same time create distraction for the reviewers. It increase the mind load of the reviewer.

Such changes should be segregated from the main change the PR addresses. There are two strategies to do so:

• separate minor changes or refactoring in the commits: this means keeping a clean commit tree and using rebase strategy so the segregation remains as the PR is updated.
• OR, create a new separate PR that addresses said minor changes. If the original PR cannot be merged until other PRs are, then it should be marked as draft until ready to merge.

Controversial Changes are Isolated

Similar to the property above, if a PR introduces several changes, with some trivial while other controversial, then the merge of all changes will be delayed.

To optimize a PR it is to separate changes that will be merged with little controversy from changes that need extensive discussion.

PR Reviewers Mandate

In return, the reviewers should ensure:

PRs are Reviewed Daily

PR should be reviewed daily to ensure that changes are promptly merged.

PR are fully reviewed

By fully reviewing a PR, the reviewers give the opportunity to the author to tackle all comments at once, maximizing the chance of the PR to be approved at the next review.

However, if there is fundamental issue with the code change that leads to a re-write of the full PR, then it is acceptable for the reviewer to not spend time reviewing code that will be modified and only highlight the fundamental issue

Expectations are Clear

Last but not least, the reviewers should clearly state their expectations from the author.

There are emoji guides that attempt to help with this. I believe a comment usually fall within five categories:

• Blocking: The current code introduces a bug or does not match what the PR should deliver; a change is expected.
• Question: Clarification requested to ensure that reviewer and author are on the same page; a reply is preferred.
• Nitpick: A change that does not impact the code behaviour, that may be purely style or preferred practice; change or reply are optional.
• Context: Some information about the piece of code or feature, to help the author acquiring knowledge about either; reply is optional.
• Future Improvement: Some tidying up, clean or logic improvements that seem to be out of the scope of the PR; may be tackled now, with a follow-up PR, tracked in an issue or discussed further for planning.

One should be able to deduce what type of comment can lead to a Request Change review result:

• Critical: This should lead to request change
• Question: While it should not lead to request change, an approval may be held off until clarifications are made.
• Nitpick/Context/Future Improvement: Should not impact the review result either way.

Which means that a reviewer marking a PR as Request Change must clearly state why with the presence of blocking comments. Similarly, a reviewer commenting on a PR without approving must clearly state why with the presence of question or blocking comments.

Conclusion

Opening or reviewing a PR is collaborating with other maintainers towards the best outcome. The same way code needs to be readable for later maintenance, a PR, or set of changes, needs to also be readable for review. Small incremental PRs will lead to faster merge and feature delivery.

On the other hand, reviewing a PR is providing feedback on said code, it is important to state expectations on comments, to not leave space to doubt for the PR author.

Comments on this article is welcome, I have am sure I forgot some corner cases but hopefully the principle are solid enough to be applicable in most scenarios.