doc: exempt test/doc only changes from 48-hr rule #16135

addaleax · 2017-10-11T03:04:21Z

If a change affects only specific parts of the tests and or
documentation, and in particular not of the runtime code itself,
it should often be okay to land it early.

The primary goal of the 48-hour rule is to ensure that people
who are invested in certain parts of Node have a reasonable chance
to weigh in based on their usage of Node; if the API is not
touched in a change, that is arguably much less of an issue.

In particular, this:

Should help avoid excessive nitpicking.
Helps newcomers, since their first PRs often touch only those areas
and a direct success is very motivating.
Helps with the amount of pull requests created by events such a
Code & Learn.

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

@nodejs/tsc

I can’t be there but if you feel it’s worth pointing to this PR in today’s TSC meeting please do so :)

Trott · 2017-10-11T05:43:45Z

COLLABORATOR_GUIDE.md

@@ -64,7 +64,8 @@ from other Collaborators. Leave at least 48 hours during the week and
 72 hours over weekends to account for international time differences
 and work schedules. Trivial changes (e.g. those which fix minor bugs
 or improve performance without affecting API or causing other
-wide-reaching impact) may be landed after a shorter delay.
+wide-reaching impact), and changes that affect only specific parts of


Nit, non-blocking: s/only specific/small/

specific may cause someone to ask "And which specific parts are those?"

(Of course "small parts" may cause someone to ask "What is small?" but the answer there is "We trust collaborator's judgment to know when a change is small or not."

If "small" is not the right word here, maybe "focused" or "localized"? (Although "localized" runs the risk of being confused with language localization.)

Maybe "small changes that affect only documentation and/or the test suite" would work?

I actually like “focused” pretty much – e.g. a PR that refactors var to let/const doesn’t need to be small but can still be focused

mcollina · 2017-10-11T14:30:57Z

COLLABORATOR_GUIDE.md

@@ -64,7 +64,8 @@ from other Collaborators. Leave at least 48 hours during the week and
 72 hours over weekends to account for international time differences
 and work schedules. Trivial changes (e.g. those which fix minor bugs
 or improve performance without affecting API or causing other
-wide-reaching impact) may be landed after a shorter delay.
+wide-reaching impact), and focused changes that affect only documentation
+and/or the test suite, may be landed after a shorter delay.


I would add "if the change is approved by at least 2 collaborators". This is the typical case anyway.

@mcollina done

mhdawson

LGTM

Fishrock123

Seems fine to me. I think I'd still prefer @mcollina's comment but I'm not certain it is necessary.

cjihrig

LGTM, but would strongly prefer @mcollina's suggestion be incorporated.

If a change affects only specific parts of the tests and or documentation, and in particular not of the runtime code itself, it should often be okay to land it early. The primary goal of the 48-hour rule is to ensure that people who are invested in certain parts of Node have a reasonable chance to weigh in based on their usage of Node; if the API is not touched in a change, that is arguably much less of an issue. In particular, this: - Should help avoid excessive nitpicking. - Helps newcomers, since their first PRs often touch only those areas and a direct success is very motivating. - Helps with the amount of pull requests created by events such a Code & Learn.

If a change affects only specific parts of the tests and or documentation, and in particular not of the runtime code itself, it should often be okay to land it early. The primary goal of the 48-hour rule is to ensure that people who are invested in certain parts of Node have a reasonable chance to weigh in based on their usage of Node; if the API is not touched in a change, that is arguably much less of an issue. In particular, this: - Should help avoid excessive nitpicking. - Helps newcomers, since their first PRs often touch only those areas and a direct success is very motivating. - Helps with the amount of pull requests created by events such a Code & Learn. PR-URL: #16135 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2017-10-12T13:20:49Z

Landed in a766f6d

If a change affects only specific parts of the tests and or documentation, and in particular not of the runtime code itself, it should often be okay to land it early. The primary goal of the 48-hour rule is to ensure that people who are invested in certain parts of Node have a reasonable chance to weigh in based on their usage of Node; if the API is not touched in a change, that is arguably much less of an issue. In particular, this: - Should help avoid excessive nitpicking. - Helps newcomers, since their first PRs often touch only those areas and a direct success is very motivating. - Helps with the amount of pull requests created by events such a Code & Learn. PR-URL: nodejs/node#16135 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

If a change affects only specific parts of the tests and or documentation, and in particular not of the runtime code itself, it should often be okay to land it early. The primary goal of the 48-hour rule is to ensure that people who are invested in certain parts of Node have a reasonable chance to weigh in based on their usage of Node; if the API is not touched in a change, that is arguably much less of an issue. In particular, this: - Should help avoid excessive nitpicking. - Helps newcomers, since their first PRs often touch only those areas and a direct success is very motivating. - Helps with the amount of pull requests created by events such a Code & Learn. PR-URL: #16135 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. labels Oct 11, 2017

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 11, 2017

addaleax requested a review from a team October 11, 2017 03:09

targos approved these changes Oct 11, 2017

View reviewed changes

Qard approved these changes Oct 11, 2017

View reviewed changes

joyeecheung approved these changes Oct 11, 2017

View reviewed changes

gireeshpunathil approved these changes Oct 11, 2017

View reviewed changes

Trott reviewed Oct 11, 2017

View reviewed changes

addaleax force-pushed the 48-hours-exempt-docs-tests branch from d9c8a19 to f6f4029 Compare October 11, 2017 06:14

lpinca approved these changes Oct 11, 2017

View reviewed changes

MylesBorins approved these changes Oct 11, 2017

View reviewed changes

mcollina reviewed Oct 11, 2017

View reviewed changes

mhdawson approved these changes Oct 11, 2017

View reviewed changes

evanlucas approved these changes Oct 11, 2017

View reviewed changes

Qard approved these changes Oct 11, 2017

View reviewed changes

Fishrock123 approved these changes Oct 11, 2017

View reviewed changes

ofrobots approved these changes Oct 11, 2017

View reviewed changes

cjihrig approved these changes Oct 11, 2017

View reviewed changes

addaleax added 2 commits October 12, 2017 02:17

[squash] add "if they have multiple approvals"

2d11995

addaleax force-pushed the 48-hours-exempt-docs-tests branch from f6f4029 to 2d11995 Compare October 12, 2017 00:19

fhinkel approved these changes Oct 12, 2017

View reviewed changes

jasnell approved these changes Oct 12, 2017

View reviewed changes

jasnell closed this Oct 12, 2017

joyeecheung mentioned this pull request Nov 8, 2017

Add a new label fast-track? #16877

Closed

joyeecheung mentioned this pull request Nov 15, 2017

doc: improve collaborator guide, doc fast-tracking process #17056

Closed

4 tasks

MylesBorins added the land-on-v6.x label Nov 16, 2017

MylesBorins mentioned this pull request Nov 21, 2017

v6.12.1 proposal #17180

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: exempt test/doc only changes from 48-hr rule #16135

doc: exempt test/doc only changes from 48-hr rule #16135

addaleax commented Oct 11, 2017

Trott Oct 11, 2017

Qard Oct 11, 2017

addaleax Oct 11, 2017

mcollina Oct 11, 2017

addaleax Oct 12, 2017

mhdawson left a comment

Fishrock123 left a comment

cjihrig left a comment

jasnell commented Oct 12, 2017

doc: exempt test/doc only changes from 48-hr rule #16135

doc: exempt test/doc only changes from 48-hr rule #16135

Conversation

addaleax commented Oct 11, 2017

Checklist

Affected core subsystem(s)

Trott Oct 11, 2017

Choose a reason for hiding this comment

Qard Oct 11, 2017

Choose a reason for hiding this comment

addaleax Oct 11, 2017

Choose a reason for hiding this comment

mcollina Oct 11, 2017

Choose a reason for hiding this comment

addaleax Oct 12, 2017

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

Fishrock123 left a comment

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

jasnell commented Oct 12, 2017