Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add merge/squash guidelines to CONTRIBUTING.rst #12672
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
Changes from all commits
68dfb2f
2befa24
cfd4356
dac2eb1
4a6364c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(within the same PR?)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, fair
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
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 thatAdd 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.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"). 😁
There was a problem hiding this comment.
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.