FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: update CONTRIBUTING.md to address editing PRs by gibfahn · Pull Request #9259 · nodejs/node · GitHub

/ node Public

doc: update CONTRIBUTING.md to address editing PRs - #9259

Merged
gibfahn merged 1 commit into
nodejs:masterfrom
gibfahn:pr-contributing
Oct 28, 2016
ConversationCommits (1)ChecksFiles changed
Merged

doc: update CONTRIBUTING.md to address editing PRs#9259
gibfahn merged 1 commit into
nodejs:masterfrom
gibfahn:pr-contributing

Conversation

gibfahn commented Oct 24, 2016

Copy link
Copy Markdown
Member
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 added the doc Issues and PRs related to the documentations. label Oct 24, 2016
Trott reviewed Oct 24, 2016
View reviewed changes
Comment thread CONTRIBUTING.md Outdated

Trott Oct 24, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Trott reviewed Oct 24, 2016
View reviewed changes
Comment thread CONTRIBUTING.md Outdated

Trott Oct 24, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

gibfahn Oct 25, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Slightly amended to specify pushing their branch to their fork.

Trott reviewed Oct 24, 2016
View reviewed changes
Comment thread CONTRIBUTING.md Outdated

Trott Oct 24, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.)

Trott reviewed Oct 24, 2016
View reviewed changes
Comment thread CONTRIBUTING.md Outdated

Trott Oct 24, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Trott reviewed Oct 24, 2016
View reviewed changes
Comment thread CONTRIBUTING.md Outdated

Trott Oct 24, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

gibfahn Oct 25, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think I used the right link, let me know.

Trott reviewed Oct 24, 2016
View reviewed changes
Comment thread CONTRIBUTING.md Outdated

Trott Oct 24, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.?

Trott reviewed Oct 24, 2016
View reviewed changes
Comment thread CONTRIBUTING.md Outdated

Trott Oct 24, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Trott reviewed Oct 24, 2016
View reviewed changes
Comment thread CONTRIBUTING.md Outdated

Trott Oct 24, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit: collaborator -> Collaborator twice here

Trott Oct 24, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Trott reviewed Oct 24, 2016
View reviewed changes
Comment thread CONTRIBUTING.md Outdated

Trott Oct 24, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit: Github -> GitHub

Trott Oct 24, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Trott requested changes Oct 24, 2016
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 force-pushed the pr-contributing branch 2 times, most recently from cea3788 to 66a809f Compare October 25, 2016 00:25

gibfahn commented Oct 25, 2016

Copy link
Copy Markdown
Member Author

@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?

Trott reviewed Oct 26, 2016
View reviewed changes
Comment thread CONTRIBUTING.md Outdated

Trott Oct 26, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Trott commented Oct 26, 2016

Copy link
Copy Markdown
Member

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 force-pushed the pr-contributing branch 2 times, most recently from cf88f49 to 9ca6aeb Compare October 27, 2016 10:59

gibfahn commented Oct 27, 2016

Copy link
Copy Markdown
Member Author

@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 commented Oct 27, 2016

Copy link
Copy Markdown
Member

Yes, still looks good to me.

doc: update CONTRIBUTING.md to address editing PRs
7c383bc 
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 merged commit 7c383bc into nodejs:master Oct 28, 2016
evanlucas pushed a commit that referenced this pull request Nov 3, 2016
doc: update CONTRIBUTING.md to address editing PRs
edd8926 
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>
evanlucas mentioned this pull request Nov 3, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Nov 18, 2016
doc: update CONTRIBUTING.md to address editing PRs
29fb34b 
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
doc: update CONTRIBUTING.md to address editing PRs
00a1782 
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
doc: update CONTRIBUTING.md to address editing PRs
d2b0cae 
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
Merged
Merged
gibfahn deleted the pr-contributing branch December 16, 2016 11:10
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
Trott Trott approved these changes
santigimeno santigimeno approved these changes
+1 more reviewer
mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL