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: update CONTRIBUTING.md to address editing PRs #9259

Merged
merged 1 commit into from
Oct 28, 2016

Conversation

gibfahn
Copy link
Member

@gibfahn gibfahn commented Oct 24, 2016

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

Add more info about the contribution process after PR submission.

  • One of the things I frequently see with new contributors is that people don't appreciate that raising a PR is only half the battle, the other half is fixing nits.
  • It's also a little discouraging to raise a PR and have changes suggested, especially if you weren't expecting it.
  • The other thing that often happens is people getting confused about how Github PRs work, I found it a bit unintuitive at first that the PR just points to your branch so you can just update your branch to update the PR. Obviously we don't want to start including Git tutorials, but this comes up often enough that I think it might be worth spelling it out.

Open to suggestions on any of this!

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 24, 2016

### Step 7: Discuss and update

You will probably get feedback or requests for changes to your Pull Request,
Copy link
Member

Choose a reason for hiding this comment

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

Nit: period/full stop instead of comma after Pull Request (and, therefore, this -> This in the next line).


When someone suggests changes, you can address them by applying them to your
branch. When your branch changes, Github will automatically update the PR.

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 something more like this?:

To make changes to an existing Pull Request, make the changes to your branch. When your branch changes, GitHub will automatically update the Pull Request.

Copy link
Member Author

Choose a reason for hiding this comment

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

Slightly amended to specify pushing their branch to their fork.

```text
$ git add any/changed/files
$ git commit --amend
$ git push --force origin my-branch
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Maybe suggest --force-with-lease? Usually not an issue because the user will be the only one working on that branch, but with the whole GitHub thing of being able to allow Collaborators from the original fork to push to your branch, maybe it's best to get people into good habits early? (Would have to update the use of --force a few lines below too.)

```

**Important:** The `git push --force` command is one of the few ways to delete
history in git, before you use it, make sure you understand the risks. If in
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Period/full stop after git and therefore before -> Before also.


**Important:** The `git push --force` command is one of the few ways to delete
history in git, before you use it, make sure you understand the risks. If in
doubt, you can always ask for guidance in the Pull Request or on IRC.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Link the word IRC to #node-dev IRC channel here the same way it is done in the main README?

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 think I used the right link, let me know.

history in git, before you use it, make sure you understand the risks. If in
doubt, you can always ask for guidance in the Pull Request or on IRC.

If your Pull Request has gone quiet, post a comment in the pull request to ping
Copy link
Member

@Trott Trott Oct 24, 2016

Choose a reason for hiding this comment

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

Nit: I don't much care whether we go with Pull Request or pull request, but please use one or the other exclusively. Both are used in this sentence. :-D

Nit: has gone quiet may be a little colloquial. Maybe something like Feel free to post a comment in the Pull Request to ping reviewers if you are awaiting an answer on something.?

### Step 8: Landing

Once your PR has been reviewed and approved by at least two Node.js
Collaborators, and as long as there is consensus (no objections from a
Copy link
Member

Choose a reason for hiding this comment

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

Only one Collaborator need to approve it. (More is better, of course. And two CTC members are required for semver-major changes.)


Once your PR has been reviewed and approved by at least two Node.js
Collaborators, and as long as there is consensus (no objections from a
collaborator or unresolved issues) a collaborator should merge the PR in for
Copy link
Member

Choose a reason for hiding this comment

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

Nit: collaborator -> Collaborator twice here

Copy link
Member

Choose a reason for hiding this comment

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

Nit: I'd remove or unresolved issues

Nit: comma after the closing parenthesis

Nit: should -> can or may or even will but probably not should

Nit: Unless we explicitly indicate somewhere that PR stands for Pull Request (and we may--I didn't look at the whole doc), stick with Pull Request instead of PR here and below.

Nit: Leave off in for you

Once your PR has been reviewed and approved by at least two Node.js
Collaborators, and as long as there is consensus (no objections from a
collaborator or unresolved issues) a collaborator should merge the PR in for
you. Github often shows the PR as `Closed` at this point, but don't worry, if
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Github -> GitHub

Copy link
Member

Choose a reason for hiding this comment

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

Nit: Comma to period/full stop and change if to If.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Content looks great to me. I left a bunch of style nits, but the only one I'd insist on is the content change to indicate that you need at least one (not two) Collaborators to approve the PR.

Thanks for doing this! I agree it is likely to be much better for newcomers.

@gibfahn gibfahn force-pushed the pr-contributing branch 2 times, most recently from cea3788 to 66a809f Compare October 25, 2016 00:25
@gibfahn
Copy link
Member Author

gibfahn commented Oct 25, 2016

@Trott I think I addressed everything. Not entirely sure (e.g. about the IRC link). Let me know if you have any other suggestions, happy to keep revising! I think it's worth bikeshedding a fair bit for docs.

Is it worth adding that Pull Requests usually are left to stew for at least 48 hours? I was going to, but I feel like there's already a tonne of information there, and I don't want to overload people too much.

In fact I was considering taking out the bit about needing two CTC LGTMs to keep it simpler.

Also maybe I should mention/explain the word LGTM in there?

**Important:** The `git push --force-with-lease` command is one of the few ways
to delete history in git. Before you use it, make sure you understand the risks.
If in doubt, you can always ask for guidance in the Pull Request or on
[IRC in the #node.js channel](https://webchat.freenode.net?channels=node.js&uio=d4).
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking the #node-dev channel rather than the #Node.js channel.

@Trott
Copy link
Member

Trott commented Oct 26, 2016

LGTM as is. I'll change my review status to Approved. Optional additional bike-shed points:

  • I meant the #node-dev IRC channel and not the #Node.js channel, but either one is probably fine.
  • On mentioning the 48 hours, I have no opinion. On the one hand, hey, it's another potentially-useful piece of information. On the other hand, too much information can make it harder for people to find the one or two pieces of information they really need. So... ¯_(ツ)_/¯
  • On removing the bit about 2 CTC approvals required for semver-major: Yes, I agree, we don't need that in this doc.
  • Defining LGTM: Probably a good idea.

@gibfahn gibfahn force-pushed the pr-contributing branch 2 times, most recently from cf88f49 to 9ca6aeb Compare October 27, 2016 10:59
@gibfahn
Copy link
Member Author

gibfahn commented Oct 27, 2016

@Trott

  • IRC channel: changed to node-dev
  • 48 hours: didn't seem necessary (on second thoughts)
  • CTC approvals: also unnecessary, removed
  • LGTM - defined (was going to add a bit saying you can use the Github approval button, but again, seems like TMI)

Still LGTY?

@Trott
Copy link
Member

Trott commented Oct 27, 2016

Yes, still looks good to me.

Add more info about the contribution process after PR submission.

PR-URL: nodejs#9259

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@gibfahn gibfahn merged commit 7c383bc into nodejs:master Oct 28, 2016
evanlucas pushed a commit that referenced this pull request Nov 3, 2016
Add more info about the contribution process after PR submission.

PR-URL: #9259

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Nov 18, 2016
Add more info about the contribution process after PR submission.

PR-URL: #9259

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Nov 18, 2016
Add more info about the contribution process after PR submission.

PR-URL: #9259

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Nov 19, 2016
Add more info about the contribution process after PR submission.

PR-URL: #9259

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
This was referenced Nov 22, 2016
@gibfahn gibfahn deleted the pr-contributing branch December 16, 2016 11:10
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants