doc: reorganize COLLABORATOR_GUIDE.md #15710

Trott · 2017-10-01T15:38:28Z

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

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

Trott · 2017-10-01T15:38:35Z

@bmeurer

gibfahn · 2017-10-01T15:47:55Z

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

bmeurer · 2017-10-01T17:04:29Z

COLLABORATOR_GUIDE.md

 * 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


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

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.

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.

lpinca · 2017-10-01T17:36:24Z

COLLABORATOR_GUIDE.md

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


Nit: multiple Refs: lines may also be added.

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

refack · 2017-10-01T20:35:08Z

COLLABORATOR_GUIDE.md

+  * 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


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

PR-URL

[Fixes]

[Refs]

Reviewed-By

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

@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 · 2017-11-11T21:53:51Z

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.

refack · 2017-11-11T22:46:10Z

doc/onboarding.md

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


Extra! Extra! Read all about it!

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

refack · 2017-11-11T22:47:20Z

doc/onboarding.md

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


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 · 2017-11-12T22:16:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11385/

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 · 2017-11-13T00:27:15Z

Landed in 5df47d0.

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 added the doc Issues and PRs related to the documentations. label Oct 1, 2017

vsemozhetbyt approved these changes Oct 1, 2017

View reviewed changes

bmeurer approved these changes Oct 1, 2017

View reviewed changes

lpinca reviewed Oct 1, 2017

View reviewed changes

lpinca approved these changes Oct 1, 2017

View reviewed changes

refack reviewed Oct 1, 2017

View reviewed changes

jasnell approved these changes Oct 2, 2017

View reviewed changes

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

Trott force-pushed the collab-guid-reorg branch 3 times, most recently from 61512a1 to 3b85c56 Compare November 11, 2017 22:43

refack reviewed Nov 11, 2017

View reviewed changes

refack approved these changes Nov 11, 2017

View reviewed changes

jasnell approved these changes Nov 12, 2017

View reviewed changes

Trott force-pushed the collab-guid-reorg branch from 3b85c56 to 7e0ed66 Compare November 12, 2017 22:15

Trott force-pushed the collab-guid-reorg branch from 7e0ed66 to 4a422be Compare November 12, 2017 22:16

Trott closed this Nov 13, 2017

evanlucas mentioned this pull request Nov 13, 2017

v9.2.0 proposal #16992

Merged

MylesBorins added land-on-v6.x labels Nov 17, 2017

gibfahn mentioned this pull request Nov 21, 2017

v8.9.2 proposal #17204

Merged

MylesBorins mentioned this pull request Nov 21, 2017

v6.12.1 proposal #17180

Merged

Trott deleted the collab-guid-reorg branch January 13, 2022 22:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: reorganize COLLABORATOR_GUIDE.md #15710

doc: reorganize COLLABORATOR_GUIDE.md #15710

Trott commented Oct 1, 2017

Trott commented Oct 1, 2017

gibfahn commented Oct 1, 2017

bmeurer Oct 1, 2017

gibfahn Oct 1, 2017

Trott Nov 11, 2017

lpinca Oct 1, 2017

Trott Nov 11, 2017

refack Oct 1, 2017

Trott Nov 11, 2017

Trott commented Nov 11, 2017

refack Nov 11, 2017

refack Nov 11, 2017

refack Nov 11, 2017

Trott commented Nov 12, 2017

Trott commented Nov 13, 2017

doc: reorganize COLLABORATOR_GUIDE.md #15710

doc: reorganize COLLABORATOR_GUIDE.md #15710

Conversation

Trott commented Oct 1, 2017

Checklist

Affected core subsystem(s)

Trott commented Oct 1, 2017

gibfahn commented Oct 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Nov 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Nov 12, 2017

Trott commented Nov 13, 2017