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

doc: Add note in CONTRIBUTING.md about force-pushing #4013

Merged
merged 2 commits into from
Jun 29, 2021

Conversation

slarse
Copy link
Collaborator

@slarse slarse commented Jun 28, 2021

IMO it's pretty confusing to leave feedback on a PR, and then have the author of that PR force-push changes to it such that the parts that were reviewed are no longer even part of the PRs commit history.

This PR adds a note to the CONTRIBUTING.md about avoiding force-pushing when collaborating on a PR (e.g. after receiving a review).

@monperrus @nharrand Thoughts on the general idea and potentially the phrasing in the docs?

@slarse slarse changed the title doc: Add note in CONTRIBUTING.md about force-pushing review: doc: Add note in CONTRIBUTING.md about force-pushing Jun 28, 2021
@I-Al-Istannen
Copy link
Collaborator

It is mentioned in passing that the PR is squashed ("Spoon integrators check the quality of the squashed merge commit message", Emphasis mine), but highlighting that in the force-push paragraph might be useful?

I normally force-push if I did something stupid and another commit will just be confusing if it ends up in the repo's history. If the PR is squashed anyways, I do not have to care about maintaining a proper history.

Explicitly stating "hey, a clean history is useful but your PR will be squashed anyways" might help a bit in alleviating the contributor's guilt :)

@slarse
Copy link
Collaborator Author

slarse commented Jun 28, 2021

@I-Al-Istannen Good point, I've included a mention of that :)

@monperrus monperrus changed the title review: doc: Add note in CONTRIBUTING.md about force-pushing doc: Add note in CONTRIBUTING.md about force-pushing Jun 29, 2021
@monperrus monperrus merged commit 5a5b7d2 into INRIA:master Jun 29, 2021
@monperrus
Copy link
Collaborator

Thanks for the clarification.

@slarse slarse deleted the please-dont-force-push branch June 29, 2021 16:02
@monperrus monperrus mentioned this pull request Aug 19, 2021
woutersmeenk pushed a commit to woutersmeenk/spoon that referenced this pull request Aug 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants