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: reorganize COLLABORATOR_GUIDE.md #15710

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 1, 2017

Based on feedback during a recent collaborator onboarding, move the
metadata description closer to where the instructions for editing a
commit message are.

Indicate which metadata are required and which are optional.

Make the punctuation more consistent. Previously, sentences prior to
instructions ended in a period, a colon, or no punctuation at all. Colon
seems best, so changed the Technical How-To section to use colons.

Checklist
Affected core subsystem(s)

doc

@Trott Trott added the doc Issues and PRs related to the documentations. label Oct 1, 2017
@Trott
Copy link
Member Author

Trott commented Oct 1, 2017

@bmeurer

@gibfahn
Copy link
Member

gibfahn commented Oct 1, 2017

I agree this info makes sense in the new location, but I think it's also useful to have it in the old, with other info about landing commits.

Maybe just give it it's own heading (so it can be linked against), and then the old location can just contain a link to the new location (or vice versa).

* The commit message text must conform to the
[commit message guidelines](./CONTRIBUTING.md#commit-message-guidelines).

* Always modify the original commit message to include additional meta
Copy link
Member

Choose a reason for hiding this comment

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

How about mentioning node-review extension here? It doesn't make sense to even try to extract this info manually.

Copy link
Member

Choose a reason for hiding this comment

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

We already mention node-review in the onboarding doc, though I'm not really sure how to reduce duplication here.

+1 on mentioning node-review here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great idea. I added a sentence and link for node-review. I also updated the contents of the onboarding doc to note that node-review works in Edge browser too. At some point soon, we'll have to add the CLI tool for this that @joyeecheung et al. have been working on.

for an issue, and/or the hash and commit message if the commit fixes
a bug in a previous commit. Multiple `Fixes:` lines may be added if
appropriate.
* Optional: A `Refs:` line referencing a URL for any relevant background.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: multiple Refs: lines may also be added.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Changed it to be "one or more".

* Required: A `PR-URL:` line that references the *full* GitHub URL of the
original pull request being merged so it's easy to trace a commit back to
the conversation that led up to that change.
* Required: A `Reviewed-By: Name <email>` line for yourself and any
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the order in the previous revision was invariant, i.e.:

  1. PR-URL
  2. [Fixes]
  3. [Refs]
  4. Reviewed-By

Anyway that's the order node-review formats the metadata.

Copy link
Member Author

Choose a reason for hiding this comment

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

@refack I didn't mean for the list to specify ordering, but since that's something you inferred and I doubt you will be alone in doing that, I've change it back to the ordering you have above.

@Trott Trott force-pushed the collab-guid-reorg branch 3 times, most recently from 9e9f472 to 4d7c723 Compare November 11, 2017 21:52
@Trott
Copy link
Member Author

Trott commented Nov 11, 2017

Maybe just give it it's own heading (so it can be linked against), and then the old location can just contain a link to the new location (or vice versa).

@gibfahn Good suggestion. I added an anchor using <a name=... (since valid HTML is valid markdown) and put in a single sentence with a link.

@Trott Trott force-pushed the collab-guid-reorg branch 3 times, most recently from 61512a1 to 3b85c56 Compare November 11, 2017 22:43
@@ -158,7 +158,7 @@ onboarding session.
* After one or two approvals, land the PR.
* Be sure to add the `PR-URL: <full-pr-url>` and appropriate `Reviewed-By:` metadata!
* [`core-validate-commit`][] helps a lot with this – install and use it if you can!
* If you use Chrome, [`node-review`][] fetches the metadata for you
* If you use Chrome or Edge, [`node-review`][] fetches the metadata for you.
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra! Extra! Read all about it!

Copy link
Contributor

Choose a reason for hiding this comment

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

New CLI tool node-core-utils fetches the needed meta data from the command line.

@@ -158,7 +158,7 @@ onboarding session.
* After one or two approvals, land the PR.
* Be sure to add the `PR-URL: <full-pr-url>` and appropriate `Reviewed-By:` metadata!
* [`core-validate-commit`][] helps a lot with this – install and use it if you can!
* If you use Chrome, [`node-review`][] fetches the metadata for you
* If you use Chrome or Edge, [`node-review`][] fetches the metadata for you.
Copy link
Contributor

Choose a reason for hiding this comment

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

New CLI tool node-core-utils fetches the needed meta data from the command line.

Based on feedback during a recent collaborator onboarding, move the
metadata description closer to where the instructions for editing a
commit message are.

Indicate which metadata are required and which are optional.

Make the punctuation more consistent. Previously, sentences prior to
instructions ended in a period, a colon, or no punctuation at all. Colon
seems best, so changed the Technical How-To section to use colons.
@Trott
Copy link
Member Author

Trott commented Nov 12, 2017

Trott added a commit to Trott/io.js that referenced this pull request Nov 13, 2017
Based on feedback during a recent collaborator onboarding, move the
metadata description closer to where the instructions for editing a
commit message are.

Indicate which metadata are required and which are optional.

Make the punctuation more consistent. Previously, sentences prior to
instructions ended in a period, a colon, or no punctuation at all. Colon
seems best, so changed the Technical How-To section to use colons.

PR-URL: nodejs#15710
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@Trott
Copy link
Member Author

Trott commented Nov 13, 2017

Landed in 5df47d0.

@Trott Trott closed this Nov 13, 2017
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
Based on feedback during a recent collaborator onboarding, move the
metadata description closer to where the instructions for editing a
commit message are.

Indicate which metadata are required and which are optional.

Make the punctuation more consistent. Previously, sentences prior to
instructions ended in a period, a colon, or no punctuation at all. Colon
seems best, so changed the Technical How-To section to use colons.

PR-URL: #15710
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@evanlucas evanlucas mentioned this pull request Nov 13, 2017
MylesBorins pushed a commit that referenced this pull request Nov 17, 2017
Based on feedback during a recent collaborator onboarding, move the
metadata description closer to where the instructions for editing a
commit message are.

Indicate which metadata are required and which are optional.

Make the punctuation more consistent. Previously, sentences prior to
instructions ended in a period, a colon, or no punctuation at all. Colon
seems best, so changed the Technical How-To section to use colons.

PR-URL: #15710
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2017
Based on feedback during a recent collaborator onboarding, move the
metadata description closer to where the instructions for editing a
commit message are.

Indicate which metadata are required and which are optional.

Make the punctuation more consistent. Previously, sentences prior to
instructions ended in a period, a colon, or no punctuation at all. Colon
seems best, so changed the Technical How-To section to use colons.

PR-URL: #15710
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@gibfahn gibfahn mentioned this pull request Nov 21, 2017
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
Based on feedback during a recent collaborator onboarding, move the
metadata description closer to where the instructions for editing a
commit message are.

Indicate which metadata are required and which are optional.

Make the punctuation more consistent. Previously, sentences prior to
instructions ended in a period, a colon, or no punctuation at all. Colon
seems best, so changed the Technical How-To section to use colons.

PR-URL: #15710
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
Based on feedback during a recent collaborator onboarding, move the
metadata description closer to where the instructions for editing a
commit message are.

Indicate which metadata are required and which are optional.

Make the punctuation more consistent. Previously, sentences prior to
instructions ended in a period, a colon, or no punctuation at all. Colon
seems best, so changed the Technical How-To section to use colons.

PR-URL: #15710
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@Trott Trott deleted the collab-guid-reorg branch January 13, 2022 22:47
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.

8 participants