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

docs,meta: assign PR semantics #23292

Merged
merged 2 commits into from
Oct 13, 2018
Merged

Conversation

refack
Copy link
Contributor

@refack refack commented Oct 6, 2018

  • some refresh to wording.

/CC the non-existing @nodejs/colab-governance (for in lieu of pinging all Collaborators) :shrung:

Refs: #23249

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@refack refack added doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. labels Oct 6, 2018
@refack refack self-assigned this Oct 6, 2018
@refack refack changed the title * docs,meta: assign PR semantics docs,meta: assign PR semantics Oct 6, 2018
Check PRs from new contributors to make sure the person's name and email address
are correct before merging.
For PRs from first time contributors it is recommended to be
[welcoming][#welcoming-first-time-contributors] and verify that the git settings
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Oct 6, 2018

Choose a reason for hiding this comment

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

[#welcoming-first-time-contributors] -> (#welcoming-first-time-contributors) (otherwise, the link is not rendered).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

@vsemozhetbyt
Copy link
Contributor

Doc format and wording LGTM with a nit.
Not sure how to feel about one more rules to observe, though it seems valid as it secures a PR author.
Should the TSC chime in?

one](https://github.com/nodejs/node/commit/b636ba8186) if unsure exactly how
to format your commit messages.
1. Add all necessary [metadata](#metadata) to commit messages before landing. If
You are unsure exactly how to format the commit messages, use the commit log
Copy link
Contributor

Choose a reason for hiding this comment

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

s/You/you/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

to format your commit messages.
1. Add all necessary [metadata](#metadata) to commit messages before landing. If
You are unsure exactly how to format the commit messages, use the commit log
as a reference, for examples commits such as [this one][commit-example].
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the sentence should end here at 'reference' and the last part re-worded. Perhaps something like 'See [this commit][commit-example] as an example.'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Oct 6, 2018

docs -> doc in commit title? It seems commmit linter is in action)

@refack refack force-pushed the colab-guide-refresh branch 2 times, most recently from 4784d75 to 3a68aaf Compare October 6, 2018 16:25
@@ -99,7 +99,8 @@ As soon as the PR is ready to land, please do so. Landing your own pull requests
allows other Collaborators to focus on other pull requests. If your pull request
is still awaiting the [minimum time to land](#waiting-for-approvals), add the
`author ready` label so other Collaborators know it can land as soon as the time
ends.
ends. If instead you wish to land the PR yourself, indicate this intent by using
the "assign yourself" button, to self assign 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.

typo: self-assign

@@ -504,12 +505,15 @@ The TSC should serve as the final arbiter where required.

## Landing Pull Requests

1. Avoid landing PRs that are assigned to someone else. Authors who wish to land
their own PRs will self assign them, or delegate to someone else.
Copy link
Member

Choose a reason for hiding this comment

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

“If in doubt, ask the assignee whether it is okay to land?”

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.

LGTM. A handful of nits/suggestions. Use as you see fit.

@@ -99,7 +99,8 @@ As soon as the PR is ready to land, please do so. Landing your own pull requests
allows other Collaborators to focus on other pull requests. If your pull request
is still awaiting the [minimum time to land](#waiting-for-approvals), add the
`author ready` label so other Collaborators know it can land as soon as the time
ends.
ends. If instead you wish to land the PR yourself, indicate this intent by using
the "assign yourself" button, to self-assign 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.

Maybe just this?:

If you wish to land the PR yourself, use the "assign yourself" button to self-assign the PR.

@@ -504,12 +505,16 @@ The TSC should serve as the final arbiter where required.

## Landing Pull Requests

1. Avoid landing PRs that are assigned to someone else. Authors who wish to land
their own PRs will self-assign them, or delegate to someone else. If in
doubt, ask the assignee whether it is okay to land?
Copy link
Member

Choose a reason for hiding this comment

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

land? -> land.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

* The "Create a merge commit" method will add an unnecessary merge commit.
* The "Squash and merge" method will add metadata (the PR #) to the commit
title, and if more than one author has contributed to the PR, squashing
will only keep the most recent author when squashing.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this?:

The "Squash and merge" method will add metadata (the PR #) to the commit title. If more than one author has contributed to the PR, squashing will only keep the most recent author.

If you don't like that and prefer the existing wording, then my comment is to please remove when squashing in squashing will only keep the most recent author when squashing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

are correct before merging.
For PRs from first time contributors it is recommended to be
[welcoming](#welcoming-first-time-contributors) and verify that the git settings
are to their liking.
Copy link
Member

@Trott Trott Oct 12, 2018

Choose a reason for hiding this comment

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

How about this?:

For PRs from first time contributors, be [welcoming](#welcoming-first-time-contributors).
Also, verify that their git settings are to their liking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

PR-URL: nodejs#23292
Refs: nodejs#23249
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs#23292
Refs: nodejs#23249
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@refack
Copy link
Contributor Author

refack commented Oct 13, 2018

@refack refack merged commit fc0da7f into nodejs:master Oct 13, 2018
@refack refack deleted the colab-guide-refresh branch October 13, 2018 22:54
addaleax pushed a commit that referenced this pull request Oct 14, 2018
PR-URL: #23292
Refs: #23249
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
addaleax pushed a commit that referenced this pull request Oct 14, 2018
PR-URL: #23292
Refs: #23249
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
PR-URL: #23292
Refs: #23249
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
PR-URL: #23292
Refs: #23249
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@refack refack removed their assignment Oct 20, 2018
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
PR-URL: #23292
Refs: #23249
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
PR-URL: #23292
Refs: #23249
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
PR-URL: #23292
Refs: #23249
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
PR-URL: #23292
Refs: #23249
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
PR-URL: #23292
Refs: #23249
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
PR-URL: #23292
Refs: #23249
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
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. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants