Skip to content

docs,meta: assign PR semantics #23292

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.

Sign up for GitHub

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

Jump to bottom
Merged
refack merged 2 commits into from
Oct 13, 2018
Merged

docs,meta: assign PR semantics #23292

refack merged 2 commits into from
Oct 13, 2018

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
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 6, 2018

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1140/pipeline

@refack refack changed the title * docs,meta: assign PR semantics docs,meta: assign PR semantics Oct 6, 2018
vsemozhetbyt
vsemozhetbyt approved these changes Oct 6, 2018
View reviewed changes
COLLABORATOR_GUIDE.md Outdated
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Oct 6, 2018 •
edited
Loading

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

@refack refack Oct 6, 2018

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

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?

mscdex
mscdex reviewed Oct 6, 2018
View reviewed changes
COLLABORATOR_GUIDE.md Outdated
Copy link
Contributor

@mscdex mscdex Oct 6, 2018

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

@refack refack Oct 6, 2018

Choose a reason for hiding this comment

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

ack

mscdex
mscdex reviewed Oct 6, 2018
View reviewed changes
COLLABORATOR_GUIDE.md Outdated
Copy link
Contributor

@mscdex mscdex Oct 6, 2018

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

@refack refack Oct 6, 2018

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 •
edited
Loading

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
addaleax
addaleax approved these changes Oct 6, 2018
View reviewed changes
COLLABORATOR_GUIDE.md Outdated
Copy link
Member

@addaleax addaleax Oct 6, 2018

Choose a reason for hiding this comment

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

typo: self-assign

COLLABORATOR_GUIDE.md Outdated
Copy link
Member

@addaleax addaleax Oct 6, 2018

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

@refack refack force-pushed the colab-guide-refresh branch from 6c2c16f to c254e71 Compare October 12, 2018 00:07
Trott
Trott approved these changes Oct 12, 2018
View reviewed changes
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.

COLLABORATOR_GUIDE.md Outdated
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.

Maybe just this?:

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

COLLABORATOR_GUIDE.md Outdated
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.

land? -> land.

Copy link
Contributor Author

@refack refack Oct 13, 2018

Choose a reason for hiding this comment

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

Ack

COLLABORATOR_GUIDE.md Outdated
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.

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

@refack refack Oct 13, 2018

Choose a reason for hiding this comment

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

Ack

COLLABORATOR_GUIDE.md Outdated
Copy link
Member

@Trott Trott Oct 12, 2018 •
edited
Loading

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

@refack refack Oct 13, 2018

Choose a reason for hiding this comment

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

Ack

@refack refack force-pushed the colab-guide-refresh branch from c254e71 to 33f7e58 Compare October 12, 2018 00:10
refack added 2 commits October 13, 2018 18:46
@refack
doc,meta: refresh wording in colab guide
39ae000 
PR-URL: nodejs#23292
Refs: nodejs#23249
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@refack
doc,meta: assign PR semantics
fc0da7f 
PR-URL: nodejs#23292
Refs: nodejs#23249
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@refack refack force-pushed the colab-guide-refresh branch from 33f7e58 to fc0da7f Compare October 13, 2018 22:46
@refack
Copy link
Contributor Author

refack commented Oct 13, 2018

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1238/

@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
@refack @addaleax
doc,meta: refresh wording in colab guide
34ebbfd 
PR-URL: #23292
Refs: #23249
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
addaleax pushed a commit that referenced this pull request Oct 14, 2018
@refack @addaleax
doc,meta: assign PR semantics
a7ed986 
PR-URL: #23292
Refs: #23249
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
@refack @jasnell
doc,meta: refresh wording in colab guide
d08544f 
PR-URL: #23292
Refs: #23249
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
@refack @jasnell
doc,meta: assign PR semantics
72258c3 
PR-URL: #23292
Refs: #23249
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@refack refack removed their assignment Oct 20, 2018
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
@refack @MylesBorins
doc,meta: refresh wording in colab guide
b7d6c1a 
PR-URL: #23292
Refs: #23249
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
@refack @MylesBorins
doc,meta: assign PR semantics
26f4221 
PR-URL: #23292
Refs: #23249
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Nov 27, 2018
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@refack @rvagg
doc,meta: refresh wording in colab guide
68b6f86 
PR-URL: #23292
Refs: #23249
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@refack @rvagg
doc,meta: assign PR semantics
d1fcdc2 
PR-URL: #23292
Refs: #23249
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
@refack @MylesBorins
doc,meta: refresh wording in colab guide
2ddf867 
PR-URL: #23292
Refs: #23249
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
@refack @MylesBorins
doc,meta: assign PR semantics
73f490a 
PR-URL: #23292
Refs: #23249
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott approved these changes

@addaleax addaleax addaleax approved these changes

@targos targos targos approved these changes

@trivikr trivikr trivikr approved these changes

@gireeshpunathil gireeshpunathil Awaiting requested review from gireeshpunathil

@BridgeAR BridgeAR Awaiting requested review from BridgeAR

+2 more reviewers

@mscdex mscdex mscdex left review comments

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@refack @nodejs-github-bot @vsemozhetbyt @mscdex @Trott @addaleax @targos @trivikr