Skip to content

Add section about expectations for merging PRs #147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Bisaloo merged 3 commits into from
Mar 24, 2025
Merged

Add section about expectations for merging PRs #147

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7674842
Add section about expectations for merging PRs
Bisaloo Mar 10, 2025
497d388
Clarify it is always maintainer regardless of who reviewed or opened
Bisaloo Mar 10, 2025
78f197e
Include suggestions and comments from review
Bisaloo Mar 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions git-branching-merging.qmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,24 @@ Because of this, we have disabled merge commits in our repositories.
Instead, we rely on either 'rebase and merge' (preferred option) if commits are grouped logically and have clear error messages, or 'squash and merge' if we don't want to keep the commit history (as a last resort; if the commit history is too messy and impossible to clean, or if there is a merge commit in the history).
If necessary and in order to make a 'rebase' rather than a 'squash', the PR history can be rewritten & cleaned using a command-line interactive git rebase, and squashing, re-ordering, rewording, etc. as necessary and as described in [this blog post](https://github.blog/2022-06-30-write-better-commits-build-better-projects/).

## Who merges the pull request?

By convention, the package maintainer is in charge of merging the pull requests.
This is indeed often more convenient as they are aware of other ongoing activities in the package that may impact or be impacted with the incoming changes.
Copy link
Contributor

@sbfnk sbfnk Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This is indeed often more convenient as they are aware of other ongoing activities in the package that may impact or be impacted with the incoming changes.
This is because they are aware of other ongoing activities in the package that may impact or be impacted with the incoming changes.

or else say more convenient than what

We also have a guarantee that maintainers have sufficient permissions to merge the pull request, and potentially bypass some checks if necessary, which may not be the case of the contributor.

This recommendation remains valid even if the maintainer if the author of the pull request and they request review from a non-maintainer.
Copy link
Member

@jamesmbaazam jamesmbaazam Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This recommendation remains valid even if the maintainer if the author of the pull request and they request review from a non-maintainer.
This recommendation remains valid even if the maintainer is the author of the pull request and they request review from a non-maintainer.


As a contributor, if you want to signal your changes are not ready to be merged, you should mark your pull requests as draft.
Copy link
Member

@jamesmbaazam jamesmbaazam Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that this is mentioned. Do we also want to consider explicitly tagging a reviewer before reviewing can begin or can people just review if they feel the need to? I think the former introduces a little bit of friction but solves the issue where a PR seems to be open for review but is not ready or is still being actively worked on. The other solution would be to only open a PR for review after it was previously set to draft.


In all cases, communication is key.
As a maintainer, if you see a non draft pull request that looks like it might still receive additional changes, please check in with the contributor before proceeding with the merge.
Conversely, if a PR is marked as draft but seems ready to go, you can check in with the contributor if they have additional changes to make or if the PR can be marked as ready for review.
As a contributor, you can also check in with the maintainer to ensure they have not simply forgotten to merge the changes, especially if they already have approved the PR.
Comment on lines +95 to +97
Copy link
Member

@jamesmbaazam jamesmbaazam Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if the author is unresponsive?

Copy link
Member Author

@Bisaloo Bisaloo Mar 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we can have a policy for this. I suspect it will have to be dealt with on a case by case basis depending on who is unresponsive, who is trying to get something merged, what the exact circumstances, etc.

If you have a specific suggestion, please propose a follow up PR.


Note that this is only an internal convention and even though we provide reasons why this may be easier, this is not intrinsically better than other conventions (e.g., PR author always merges or reviewer always merges).
The main goal is to clarify expectations to streamline process and avoid uncertainty-induced action paralysis, where everyone is waiting for the others to merge the PR.

## Deleting merged branches

To avoid confusing situations with diverging histories, we prefer to delete branches.
Expand Down