Skip to content
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.

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

Add merge/squash guidelines to CONTRIBUTING.rst #12672

Merged
merged 5 commits into from
Aug 6, 2024
Merged
Changes from 1 commit
Commits
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
54 changes: 53 additions & 1 deletion CONTRIBUTING.rst
nicoddemus marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,56 @@ pull requests from other contributors yourself after having reviewed
them.


Merge/squash guidelines
-----------------------

When a PR is approved and ready to be integrated to the `main` branch, one has the option to *merge* the commits unchanged, or *squash* all the commits into a single commit.

Here is some guidelines on how to proceeed, based on the state of the PR commit history:
nicoddemus marked this conversation as resolved.
Show resolved Hide resolved

1. Miscellaneous commits:
Copy link
Member

@webknjaz webknjaz Jul 31, 2024

Choose a reason for hiding this comment

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

(within the same PR?)

Copy link
Member Author

@nicoddemus nicoddemus Jul 31, 2024

Choose a reason for hiding this comment

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

Yes, I did change the introductory phrase just before the examples in an attempt to make it clearer.

Copy link
Member

Choose a reason for hiding this comment

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

okay, fair


* `Implement X`
* `Fix test_a`
* `Add myself to AUTHORS`
* `!fixup Fix test_a`
* `Update tests/test_integration.py`
* `Update tests/test_integration.py`
nicoddemus marked this conversation as resolved.
Show resolved Hide resolved

In this case, prefer to use the **Squash** merge strategy: the commit history is a bit messy (not in a derrogatory way, often one just commits changes because they know the changes will eventually be squashed together), so squashing everything into a single commit is best. Prefer to also edit the default GitHub message to add more details and improve on it.
nicoddemus marked this conversation as resolved.
Show resolved Hide resolved

2. Separate commits related to the same topic:

* `Implement X`
* `Add myself to AUTHORS`
* `Update CHANGELOG for X`

In this case, prefer to use the **Squash** merge strategy: while the commit history is not "messy" as in the example above, the individual commits do not bring much value overall, specially when looking at the changes a few months/years down the line.
Copy link
Member

Choose a reason for hiding this comment

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

How about “Add tests for X”? Such commits totally deserve to be revertible separately...

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry but again we disagree: I don't see the point of keeping the tests if we are reverting the feature? And vice versa. And even if/when that happens (which TBH I have never seen happen in pytest's entire history) I guess we can cherry-pick/edit manually -- but given this is really rare, I don't think we need an exception/rule for this.

Copy link
Member

Choose a reason for hiding this comment

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

This is quite a controversial example overall. It's not only about the tests (which might need to be reverted because they were relying on incorrect expectations, for example). Even with adding oneself to the authors. Imagine removing the entire thing, but a person contributed more things after that first PR, and you'd just be deleting their credit while it would be better to keep it. Same with the change notes. Although, with these, I'd argue that they should never be reverted even if the feature is, since the contribution has been made even if its code has been modified/rewritten/removed at some point. The same goes for changelog entries — if something was implemented in 8.2 and reverted in 8.3, it doesn't mean that the change log should eliminate any mentions of it from historic documents.

Copy link
Member Author

Choose a reason for hiding this comment

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

Even with adding oneself to the authors. Imagine removing the entire thing, but a person contributed more things after that first PR, and you'd just be deleting their credit while it would be better to keep it.

Contributions are kept (unless someone explicitly removes them) via the Co-authored-by field, for an example:

d489247

I understand your examples, but they seem to me more an exception rather than something which happens all the time and hence we should always merge (instead of squashing).

Copy link
Member

Choose a reason for hiding this comment

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

I'm talking about the AUTHORS file, which is what that Add myself to AUTHORS example commit message implies. Git commits are of course kept in history, I know that.

And yes, my point is that natural merge is better here.

Copy link
Member Author

@nicoddemus nicoddemus Aug 1, 2024

Choose a reason for hiding this comment

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

I understand your point, but I still think this is not a good approach because these reverts we are discussing are incredibly rare in my experience (both in pytest and in other repositories).

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps. Though, being rare, it makes it harder for people to remember to make partial reverts.


3. Separate commits, each with their own topic (refactorings, renames, etc), but still have a larger topic/purpose.

* `Refactor class X in preparation for feature Y`
* `Remove unused method`
* `Implement feature Y`

In this case, prefer to use the **Merge** strategy: each commit is valuable on its own, even if they serve a common topic overall. Looking at the history later, it is useful to have the removal of the unused method separately on its own commit, along with more information (such as how it became unused in the first place).

4. Separate commits, each with their own topic, but without a larger topic/purpose other than improve the code base (using more modern techniques, improve typing, removing clutter, etc).
Copy link
Member

Choose a reason for hiding this comment

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

Should we note somewhere that such PRs might not be backportable? (since those are squashed)

Copy link
Member Author

@nicoddemus nicoddemus Jul 31, 2024

Choose a reason for hiding this comment

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

Not sure what you mean, can you elaborate? In the example 4 above the recommendation is to merge the PR, so individual commits might still be backported.

Copy link
Member

Choose a reason for hiding this comment

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

@nicoddemus there is no mechanism for automatically backporting individual commits. They can only be backported together as a whole this way.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm aware, not sure what you meant by your first comment? That example 4 above does not even mention backports...

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't mention backports, but it says that unrelated commits can be in one PR. And applying backport labels to such PRs will inevitably offer backports where those are squashed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see... want to make a suggestion then?

Copy link
Member

Choose a reason for hiding this comment

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

I'd normally suggest not making such PRs. Asking people to remember to pick out individual commits when backporting is an extra maintenance burden. I don't see how that could work realistically.

Copy link
Member

Choose a reason for hiding this comment

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

OTOH, maybe this could be a FR for Patchback — have a label for communicating such an intent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry I meant a direct suggestion to the text (as in "```suggestion"). 😁

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a nice phrasing in my mind for this right now. Also, I cannot imagine such a PR existing with a good description. Still, if it does have a good description, maybe it's not that bad if a bot would smash everything together in stable branches.


* `Improve internal names in X`
* `Add type annotations to Y`
* `Remove unnecessary dict access`
* `Remove unreachable code due to EOL Python`

In this case, prefer to use the **Merge** strategy: each commit is valuable on its own, and the information on each is valuable in the long term.


As mentioned, those are overall guidelines, not rules cast in stone. This topic was discussed in [#12633](https://github.com/pytest-dev/pytest/discussions/12633).
nicoddemus marked this conversation as resolved.
Show resolved Hide resolved


*Backports* should always be **squashed**, as they preserve the original PR author.
nicoddemus marked this conversation as resolved.
Show resolved Hide resolved


Backporting bug fixes for the next patch release
------------------------------------------------

Expand Down Expand Up @@ -438,6 +488,8 @@ above?
All the above are not rules, but merely some guidelines/suggestions on what we should expect
about backports.

Backports should be **squashed** (rather than **merged**), as doing so preserves the original PR author correctly.
nicoddemus marked this conversation as resolved.
Show resolved Hide resolved

Handling stale issues/PRs
-------------------------

Expand Down Expand Up @@ -485,7 +537,7 @@ When closing a Pull Request, it needs to be acknowledging the time, effort, and

<bye>

Closing Issues
Closing issues
--------------

When a pull request is submitted to fix an issue, add text like ``closes #XYZW`` to the PR description and/or commits (where ``XYZW`` is the issue number). See the `GitHub docs <https://help.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword>`_ for more information.
Expand Down
Loading