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.

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

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

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

@addaleax 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 nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 11, 2017
@addaleax addaleax requested a review from a team October 11, 2017 03:09
Trott
Trott reviewed Oct 11, 2017
View reviewed changes
COLLABORATOR_GUIDE.md Outdated
@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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
mcollina reviewed Oct 11, 2017
View reviewed changes
COLLABORATOR_GUIDE.md Outdated
@@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@mcollina done

mhdawson
mhdawson approved these changes Oct 11, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

Fishrock123
Fishrock123 approved these changes Oct 11, 2017
View reviewed changes
Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

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

cjihrig
cjihrig approved these changes Oct 11, 2017
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

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

@addaleax
doc: exempt test/doc only changes from 48-hr rule
109bc87 
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.
@addaleax
[squash] add "if they have multiple approvals"
2d11995
jasnell pushed a commit that referenced this pull request Oct 12, 2017
@addaleax @jasnell
doc: exempt test/doc only changes from 48-hr rule
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: #16135
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Oct 12, 2017

Landed in a766f6d

@jasnell jasnell closed this Oct 12, 2017
addaleax added a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
@addaleax
doc: exempt test/doc only changes from 48-hr rule
ffc0e3a 
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 <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Oct 18, 2017
@addaleax @targos
doc: exempt test/doc only changes from 48-hr rule
30afa07 
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 <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Oct 18, 2017
@addaleax @targos
doc: exempt test/doc only changes from 48-hr rule
0d23412 
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 <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@joyeecheung joyeecheung mentioned this pull request Nov 8, 2017
Closed
@joyeecheung joyeecheung mentioned this pull request Nov 15, 2017
Closed
4 tasks
MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
@addaleax @MylesBorins
doc: exempt test/doc only changes from 48-hr rule
8531d36 
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 <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
@addaleax @MylesBorins
doc: exempt test/doc only changes from 48-hr rule
ff7c75d 
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 <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
@addaleax @MylesBorins
doc: exempt test/doc only changes from 48-hr rule
281023b 
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 <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott left review comments

@mcollina mcollina mcollina left review comments

@cjihrig cjihrig cjihrig approved these changes

@targos targos targos approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@MylesBorins MylesBorins MylesBorins approved these changes

@jasnell jasnell jasnell approved these changes

@Qard Qard Qard approved these changes

@fhinkel fhinkel fhinkel approved these changes

@ofrobots ofrobots ofrobots approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@mhdawson mhdawson mhdawson approved these changes

@lpinca lpinca lpinca approved these changes

@evanlucas evanlucas evanlucas approved these changes

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.