Add merge/squash guidelines to CONTRIBUTING.rst #12672
Conversation
As discussed in pytest-dev#12633.
nicoddemus
added
skip news
|
|
||
| Here is some guidelines on how to proceeed, based on the state of the PR commit history: | ||
|
|
||
| 1. Miscellaneous commits: |
There was a problem hiding this comment.
Yes, I did change the introductory phrase just before the examples in an attempt to make it clearer.
| * `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. |
There was a problem hiding this comment.
How about “Add tests for X”? Such commits totally deserve to be revertible separately...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Perhaps. Though, being rare, it makes it harder for people to remember to make partial reverts.
|
|
||
| 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). |
There was a problem hiding this comment.
Should we note somewhere that such PRs might not be backportable? (since those are squashed)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@nicoddemus there is no mechanism for automatically backporting individual commits. They can only be backported together as a whole this way.
There was a problem hiding this comment.
I'm aware, not sure what you meant by your first comment? That example 4 above does not even mention backports...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I see... want to make a suggestion then?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
OTOH, maybe this could be a FR for Patchback — have a label for communicating such an intent.
There was a problem hiding this comment.
Sorry I meant a direct suggestion to the text (as in "```suggestion"). 😁
There was a problem hiding this comment.
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.
|
Thanks @webknjaz for the review, appreciate it. |
|
Anything else folks? Otherwise I would like to merge this. 👍 |
Backport to 8.3.x: 💚 backport PR created✅ Backport PR branch: Backported as #12692 🤖 @patchback |
As discussed in https://github.com/pytest-dev/pytest/discussions/12633. (cherry picked from commit 2b99703)
As discussed in https://github.com/pytest-dev/pytest/discussions/12633. (cherry picked from commit 2b99703) Co-authored-by: Bruno Oliveira <nicoddemus@gmail.com>
As discussed in #12633.