doc: clarify the review and landing process by joyeecheung · Pull Request #10202 · nodejs/node

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

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

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

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

The indentation is off here and possibly other places below.

Addressed. Thanks for pointing this out.

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

CONTRIBUTING.md

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

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

links to the pull request and relevant issues

Thanks, I will add this to the sentence.

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

CONTRIBUTING.md

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

Uh oh!

Conversation

joyeecheung commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

mscdex Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung Dec 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Dec 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Dec 11, 2016

Uh oh!

joyeecheung commented Dec 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Dec 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibfahn left a comment

Choose a reason for hiding this comment

Uh oh!

gibfahn commented Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

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