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: clarify the review and landing process #10202

Closed
wants to merge 10 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Dec 9, 2016

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

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. lts-watch-v4.x labels Dec 9, 2016
@joyeecheung joyeecheung changed the title doc: clarify the review process doc: clarify the review and landing process Dec 9, 2016
@@ -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
Copy link
Contributor

@mscdex mscdex Dec 9, 2016

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that would sound better.

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds better I think.

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Notes added. Thanks for the suggestion!

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
Copy link
Contributor

Choose a reason for hiding this comment

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

The indentation is off here and possibly other places below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed. Thanks for pointing this out.

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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@joyeecheung joyeecheung Dec 10, 2016

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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

@@ -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).
Copy link
Contributor

Choose a reason for hiding this comment

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

links to the pull request and relevant issues

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I will add this to the sentence.

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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

- Mention the "landed in <sha>" comment when a PR gets landed
@joyeecheung
Copy link
Member Author

joyeecheung commented Dec 11, 2016

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
Copy link
Member Author

Ping. Is there anything that needs to be addressed?


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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

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

Still LGTM

@gibfahn
Copy link
Member

gibfahn commented Dec 22, 2016

Landed in 44b38bb

Thanks a lot @joyeecheung

@gibfahn gibfahn closed this Dec 22, 2016
gibfahn pushed a commit that referenced this pull request Dec 22, 2016
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>
@joyeecheung joyeecheung deleted the contributing-guide branch January 2, 2017 05:28
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
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>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
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>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
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>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
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>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
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>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
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>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
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>
This was referenced Jan 24, 2017
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
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>
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
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>
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.