doc: clarify the review and landing process #10202

joyeecheung · 2016-12-09T19:19:45Z

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

Description of change

A few process-related details are only explained in the onboarding guide and the collaborator's guide. Mentioning them in the contributing guide as well can avoid confusions to new contributors.
This is based on my previous experience, but I am still a new contributor myself, so feel free to correct me if my understanding is not the case!

Explains what "nits" stand for
Explains commit squashing
Mention the CI run
Mention the mandatory 48/72 hours wait

Ref: #10151

mscdex · 2016-12-09T19:25:54Z

CONTRIBUTING.md

@@ -245,13 +251,27 @@ If in doubt, you can always ask for guidance in the Pull Request or on
 Feel free to post a comment in the Pull Request to ping reviewers if you are
 awaiting an answer on something.

+Notes: if the reviewer mentions *nits* in their comments, that stands for


I don't think "stands for" is the right wording. That makes it sound like 'nits' is an acronym, which it isn't.

Thank you for pointing this out. I'm not a native speaker so this is really helpful. Does "means" sounds better?

Yes, that would sound better.

mscdex · 2016-12-09T19:35:05Z

CONTRIBUTING.md

@@ -245,13 +251,27 @@ If in doubt, you can always ask for guidance in the Pull Request or on
 Feel free to post a comment in the Pull Request to ping reviewers if you are
 awaiting an answer on something.

+Notes: if the reviewer mentions *nits* in their comments, that stands for
+"requests for small changes that are not essential". Usually these nits are


I'm not sure the description is quite accurate. IMHO nits are just minor issues, whether the changes needed to address the minor issue are large or small is irrelevant. For example, comment typos for a PR that changes code would probably be considered a nit especially because comments don't affect the code, but it could be there are a lot of typos to fix or just one.

Hmm.. I copied this phrase from the onboarding guide, maybe that one needs to be more accurate too?

How does "request for changes that are not essential" sound?

That sounds better I think.

mscdex · 2016-12-09T19:36:47Z

CONTRIBUTING.md

-there is consensus (no objections from a Collaborator), a
-Collaborator can merge the Pull Request . GitHub often shows the Pull Request as
+A Pull Request needs to be reviewed and approved by at least one Node.js
+Collaborators (often by saying LGTM, or Looks Good To Me) to get landed.


It might be worth also noting Github's PR review mechanism too if we want to advocate its usage.

Does adding a link to the video tutorial(https://www.youtube.com/watch?v=HW0RPaJqm4g) and the documentation(https://help.github.com/articles/reviewing-changes-in-pull-requests/) suffice?

That might be a good start.

I think we may want to add some text about the validity of approvals (either via 'LGTM' or Github's PR review) after changes were made since the approval. For example, when someone approves a PR using Github's mechanism, it will still show "Approved" even after someone pushes more changes after that approval is made, which can be misleading (at least at first glance).

Notes added. Thanks for the suggestion!

mscdex · 2016-12-09T19:37:43Z

CONTRIBUTING.md

+But, if there are non-trivial changes in this Pull Request, it still
+needs to wait for at least another 48 hours (72 hours on a weekend).
+
+GitHub often shows the Pull Request as
 `Closed` at this point, but don't worry. If you look at the branch you raised


The indentation is off here and possibly other places below.

Addressed. Thanks for pointing this out.

gibfahn · 2016-12-10T00:32:05Z

CONTRIBUTING.md

+one commit, with metadata added to the commit message (including links to the
+Pull Request and the names of the reviewers). This will be done by someone who
+has the commit access to the repository. The commit history of your Pull
+Request, however, will stay intact on the Pull Request page.


This isn't actually true, the commit history of your PR is available until you delete your fork branch, at which point it disappears.

Thank you for pointing this out. Maybe "The commit history of your Pull Request, however, will stay intact on the Pull Request page(as long as you don't delete your fork branch, at which point it disappears)." is more accurate?

gibfahn · 2016-12-10T00:32:17Z

CONTRIBUTING.md

@@ -237,6 +237,12 @@ $ git commit --amend
 $ git push --force-with-lease origin my-branch
 ```

+When the commits in your Pull Request get landed, they will be squashed into
+one commit, with metadata added to the commit message (including links to the


The commits won't necessarily be squashed into one commit per PR, they'll be squashed into one commit per logical change (it's hard to specify what a logical change is).

Hmmm..maybe suggesting the new contributors checkout the previous commits to have an idea about the size of a logical change? Or a link to a specific example(preferably one with lib, src, doc, test changes but is still one logical change)?

I think this one could be a good example. #2921 Although it's two logical changes.

Ooops, strike that, that one landed as two commits too. I will try to dig up a better example(any help would be appreciated!).

OK, I think this commit 0b5191f could be a good example. There are a lot of lines changed, but it's still one logical change.

joyeecheung · 2016-12-10T07:00:28Z

Improved this PR with requests from @gibfahn and @mscdex. Thanks for the review!

thefourtheye · 2016-12-10T18:24:26Z

CONTRIBUTING.md

@@ -237,6 +237,19 @@ $ git commit --amend
 $ git push --force-with-lease origin my-branch
 ```

+When the commits in your Pull Request get landed, they will be squashed into
+one commit per logical change, with metadata added to the commit message
+(including links to the Pull Request and the names of the reviewers).


links to the pull request and relevant issues

Thanks, I will add this to the sentence.

thefourtheye · 2016-12-10T18:26:18Z

CONTRIBUTING.md

- your Pull Request against (probably `master`), you should see a commit with
- your name on it. Congratulations and thanks for your contribution!
+A Pull Request needs to be reviewed and approved by at least one Node.js
+Collaborators (often by saying LGTM, or Looks Good To Me) to get landed.


Collaborator? Also, collaborators can use GitHub's Approve button.

Do you mean this should be singular? I will add a phrase mentioning the Approve button, thanks

- Explains what "nits" stand for - Explains commit squashing - Mentions the CI run - Mentions the mandatory 48/72 hours wait - Mention GitHub's PR review feature - Fix indentation

joyeecheung · 2016-12-11T09:36:13Z

@thefourtheye Thank you for the review, I've updated the changes, PTAL.

- Mention the "landed in <sha>" comment when a PR gets landed

joyeecheung · 2016-12-11T09:46:36Z

I have read this one more time, and I think the comment "landed in <sha>" used by collaborators when a PR gets landed should be mentioned as well.

joyeecheung · 2016-12-22T03:25:49Z

Ping. Is there anything that needs to be addressed?

Trott · 2016-12-22T05:18:58Z

CONTRIBUTING.md

+
+A Pull Request needs to stay open for at least 48 hours (72 hours on a
+weekend) from when it is submitted, even after it gets approved and
+passes the CI. This is to make sure that everyone has a chance to 


Microscopic nit: There is a trailing space at the end of this line.

Should have trimmed the spaces :P. Addressed. Thanks!

gibfahn

Still LGTM

gibfahn · 2016-12-22T15:55:55Z

Landed in 44b38bb

Thanks a lot @joyeecheung

Adds/mentions: - Link to glossary - Commit squashing and CI run - 48/72 hour wait and PR review feature - Extra notes section - "Landed in <sha>" comment PR-URL: #10202 Ref: #10151 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

nodejs-github-bot added doc Issues and PRs related to the documentations. lts-watch-v4.x labels Dec 9, 2016

joyeecheung changed the title ~~doc: clarify the review process~~ doc: clarify the review and landing process Dec 9, 2016

joyeecheung force-pushed the contributing-guide branch from a11a8b6 to c4ff032 Compare December 9, 2016 19:22

nodejs-github-bot added lts-watch-v4.x labels Dec 9, 2016

mscdex reviewed Dec 9, 2016

View reviewed changes

gibfahn reviewed Dec 10, 2016

View reviewed changes

nodejs-github-bot added the dont-land-on-v7.x label Dec 10, 2016

thefourtheye reviewed Dec 10, 2016

View reviewed changes

addaleax removed the dont-land-on-v7.x label Dec 10, 2016

nodejs-github-bot added the dont-land-on-v7.x label Dec 11, 2016

doc: clarify the review and landing process

cc22d35

- Explains what "nits" stand for - Explains commit squashing - Mentions the CI run - Mentions the mandatory 48/72 hours wait - Mention GitHub's PR review feature - Fix indentation

joyeecheung force-pushed the contributing-guide branch from a248e7d to cc22d35 Compare December 11, 2016 09:33

nodejs-github-bot added the dont-land-on-v7.x label Dec 11, 2016

doc: clarify the landing process

e8153b7

- Mention the "landed in <sha>" comment when a PR gets landed

nodejs-github-bot added the dont-land-on-v7.x label Dec 11, 2016

Fishrock123 removed the dont-land-on-v7.x label Dec 12, 2016

joyeecheung mentioned this pull request Dec 13, 2016

buffer: throw a consistent range error for length > kMaxLength #10152

Closed

nodejs-github-bot added lts-watch-v4.x labels Dec 13, 2016

joyeecheung force-pushed the contributing-guide branch from 023d78e to 86d6d7b Compare December 13, 2016 05:59

nodejs-github-bot added the lts-watch-v4.x label Dec 13, 2016

nodejs-github-bot added lts-watch-v6.x labels Dec 20, 2016

Trott reviewed Dec 22, 2016

View reviewed changes

doc: trim trailling spaces

41e4e93

nodejs-github-bot added lts-watch-v6.x labels Dec 22, 2016

gibfahn approved these changes Dec 22, 2016

View reviewed changes

gibfahn closed this Dec 22, 2016

joyeecheung deleted the contributing-guide branch January 2, 2017 05:28

evanlucas mentioned this pull request Jan 3, 2017

v7.4.0 release proposal #10589

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jan 23, 2017

This was referenced Jan 24, 2017

v4.8.0 proposal #10973

Merged

v6.10.0 proposal #10974

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: clarify the review and landing process #10202

doc: clarify the review and landing process #10202

joyeecheung commented Dec 9, 2016 •

edited

Loading

mscdex Dec 9, 2016 •

edited

Loading

joyeecheung Dec 9, 2016

mscdex Dec 9, 2016

mscdex Dec 9, 2016

joyeecheung Dec 9, 2016

mscdex Dec 9, 2016

mscdex Dec 9, 2016

joyeecheung Dec 9, 2016

mscdex Dec 9, 2016

joyeecheung Dec 10, 2016

mscdex Dec 9, 2016

joyeecheung Dec 10, 2016

gibfahn Dec 10, 2016

joyeecheung Dec 10, 2016

gibfahn Dec 10, 2016

joyeecheung Dec 10, 2016

joyeecheung Dec 10, 2016 •

edited

Loading

joyeecheung Dec 10, 2016

joyeecheung Dec 10, 2016

joyeecheung commented Dec 10, 2016

thefourtheye Dec 10, 2016

joyeecheung Dec 11, 2016

thefourtheye Dec 10, 2016

joyeecheung Dec 11, 2016

joyeecheung commented Dec 11, 2016

joyeecheung commented Dec 11, 2016 •

edited

Loading

joyeecheung commented Dec 22, 2016

Trott Dec 22, 2016

joyeecheung Dec 22, 2016

gibfahn left a comment

gibfahn commented Dec 22, 2016 •

edited

Loading

doc: clarify the review and landing process #10202

doc: clarify the review and landing process #10202

Conversation

joyeecheung commented Dec 9, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

mscdex Dec 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung Dec 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Dec 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Dec 11, 2016

joyeecheung commented Dec 11, 2016 • edited Loading

joyeecheung commented Dec 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn left a comment

Choose a reason for hiding this comment

gibfahn commented Dec 22, 2016 • edited Loading

joyeecheung commented Dec 9, 2016 •

edited

Loading

mscdex Dec 9, 2016 •

edited

Loading

joyeecheung Dec 10, 2016 •

edited

Loading

joyeecheung commented Dec 11, 2016 •

edited

Loading

gibfahn commented Dec 22, 2016 •

edited

Loading