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: remove squash guideline from onboarding doc #20413

Closed
wants to merge 3 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Apr 30, 2018

Although I agree with the guideline people should generally not squash
commits in a pull request until the end (in other words, until it's time
to land the PR), it is clear from comments and actions in the issue
tracker that many do not share that view. This is fine by me, but I do
think that we should our documentation should reflect our practices
rather than being an aspirational statement.

If we do wish to preserve this recommendation, it probably belongs in
another document anyway as this is not a recommendation for
Collaborators only but for anyone opening a pull request.

Checklist

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Apr 30, 2018
@Trott
Copy link
Member Author

Trott commented Apr 30, 2018

@Trott
Copy link
Member Author

Trott commented Apr 30, 2018

Some recent examples of Collaborators squashing or asking others to squash before the end of work. Again, I don't have a problem with this practice. I've certainly done it myself many times. I just don't think we should have our docs say one thing and then ask people to do something different. Easiest thing to do is to remove the one sentence from our docs which is what this PR does.

@Trott
Copy link
Member Author

Trott commented Apr 30, 2018

@vsemozhetbyt vsemozhetbyt changed the title doc: remove squash guidelinefrom onboarding doc doc: remove squash guideline from onboarding doc Apr 30, 2018
@vsemozhetbyt vsemozhetbyt added the meta Issues and PRs related to the general management of the project. label Apr 30, 2018
Although I agree with the guideline people should generally not squash
commits in a pull request until the end (in other words, until it's time
to land the PR), it is clear from comments and actions in the issue
tracker that many do not share that view. This is fine by me, but I do
think that we should our documentation should reflect our practices
rather than being an aspirational statement.

If we *do* wish to preserve this recommendation, it probably belongs in
another document anyway as this is not a recommendation for
Collaborators only but for anyone opening a pull request.
@Trott
Copy link
Member Author

Trott commented Apr 30, 2018

@BridgeAR
Copy link
Member

I am -1 on removing this. I personally think we should really stick to it. Removing the comment does not help. Instead, we could go ahead and elaborate when exactly it might make sense to ask for squashing (because there are exceptions to the rule) and how it should be done in the end.

Such exceptions could for example be:

  • many commits where it is difficult to tell what exactly should stay in the commit message in the end.
  • there is a merge commit and there are no other open comments.
  • ...

@Trott
Copy link
Member Author

Trott commented Apr 30, 2018

Tell the contributor to generally not squash commits during the pull
request review process.
@Trott
Copy link
Member Author

Trott commented Apr 30, 2018

@Trott
Copy link
Member Author

Trott commented Apr 30, 2018

@BridgeAR PTAL

@BridgeAR
Copy link
Member

I am fine with the addition but I would rather keep it in onboarding as well.

@Trott
Copy link
Member Author

Trott commented May 1, 2018

I am fine with the addition but I would rather keep it in onboarding as well.

@BridgeAR How about if instead of the onboarding docs, and along with the addition to pull-requests.md, I also add a sentence in the COLLABORATOR_GUIDE.md? Would that work for you?

Trott added a commit to Trott/io.js that referenced this pull request May 3, 2018
Although I agree with the guideline people should generally not squash
commits in a pull request until the end (in other words, until it's time
to land the PR), it is clear from comments and actions in the issue
tracker that many do not share that view. This is fine by me, but I do
think that we should our documentation should reflect our practices
rather than being an aspirational statement.

If we *do* wish to preserve this recommendation, it probably belongs in
another document anyway as this is not a recommendation for
Collaborators only but for anyone opening a pull request.

PR-URL: nodejs#20413
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Trott added a commit to Trott/io.js that referenced this pull request May 3, 2018
Tell the contributor to generally not squash commits during the pull
request review process.

PR-URL: nodejs#20413
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented May 3, 2018

Landed in 7588cea...24a5ac7

If we want to add the information to COLLABORATOR_GUIDE.md or onboarding.md, that can be done in a subsequent PR.

@Trott Trott closed this May 3, 2018
MylesBorins pushed a commit that referenced this pull request May 4, 2018
Although I agree with the guideline people should generally not squash
commits in a pull request until the end (in other words, until it's time
to land the PR), it is clear from comments and actions in the issue
tracker that many do not share that view. This is fine by me, but I do
think that we should our documentation should reflect our practices
rather than being an aspirational statement.

If we *do* wish to preserve this recommendation, it probably belongs in
another document anyway as this is not a recommendation for
Collaborators only but for anyone opening a pull request.

PR-URL: #20413
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 4, 2018
Tell the contributor to generally not squash commits during the pull
request review process.

PR-URL: #20413
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
MylesBorins pushed a commit that referenced this pull request May 8, 2018
Although I agree with the guideline people should generally not squash
commits in a pull request until the end (in other words, until it's time
to land the PR), it is clear from comments and actions in the issue
tracker that many do not share that view. This is fine by me, but I do
think that we should our documentation should reflect our practices
rather than being an aspirational statement.

If we *do* wish to preserve this recommendation, it probably belongs in
another document anyway as this is not a recommendation for
Collaborators only but for anyone opening a pull request.

PR-URL: #20413
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 8, 2018
Tell the contributor to generally not squash commits during the pull
request review process.

PR-URL: #20413
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott Trott deleted the squash branch January 13, 2022 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants