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

meta: allow vague objections to be dismissed #15233

Closed
wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Sep 6, 2017

Explicitly allow vague objections to change requests to be dismissed if requests for clarification go unanswered

/cc @nodejs/collaborators

Explicitly allow vague objections to change requests to
be dismissed if requests for clarification go unanswered
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Sep 6, 2017
Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

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

Seems reasonable

rmg
rmg previously requested changes Sep 6, 2017
Copy link
Contributor

@rmg rmg left a comment

Choose a reason for hiding this comment

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

This is an example vague objection. It should be dismissed.
Edited after being dismissed.

@rmg rmg dismissed their stale review September 6, 2017 22:59

Super vague.

@benjamingr
Copy link
Member

Maybe 48 hours as a time to respond when asked to clarify?

@jasnell
Copy link
Member Author

jasnell commented Sep 7, 2017

@benjamingr ... I'd generally prefer not to give a specific time frame as what may be considered "reasonable" may vary depending on what the PR is changing

@Trott
Copy link
Member

Trott commented Sep 7, 2017

I see this has a lot of approval, but I do wish to point out that we do already have a mechanism for overriding blocked PRs: Put it on the TSC agenda for a vote. If the TSC were getting a bunch of these every month and having to rubber-stamp PRs, then I could see the reason for wanting to add something like this. But this doesn't happen much AFAIK.

This is not enough for me to object to this, but putting it out there for thought in case someone else thinks maybe it is. I'm kinda -0 on it.

@Trott
Copy link
Member

Trott commented Sep 7, 2017

Also I fear this might get abused to dismiss objections that aren't vague. Maybe add an additional sentence and some typography to drive the point home:

If any Collaborator objects to a change without giving any additional
explanation or context
, and the objecting Collaborator fails to respond to
explicit requests for explanation or context within a reasonable period of
time, the objection may be dismissed. Note that this does not apply to objections
that are explained.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@jasnell
Copy link
Member Author

jasnell commented Sep 7, 2017

@Trott ... The main goal of this is to reduce the need for frustratingly vague blocks on a pr from having to go through that more formal process and to encourage collaborators to be more cooperative in working through objections.

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

explanation or context*, and the objecting Collaborator fails to respond to
explicit requests for explanation or context within a reasonable period of
time, the objection may be dismissed. Note that this does not apply to
objections that are explained.
Copy link
Member

Choose a reason for hiding this comment

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

I actually went ahead and dismissed some reviews that were highly outdated after pinging the persons multiple times plus always asking them to do a new review. Most of them never replied at all and otherwise fine PRs stalled because of that even though it had lots of LGs. Those were mainly with explanation and context and the comments were addressed properly or it was clear after a discussion that something would not apply. So this is mainly not exactly the same but I fear that limiting this with the last sentence is a bad idea.

I know @Trott feared abuse but I personally think that anyone who is actually a collaborator should uphold a high standard and if that is not the case the other collaborators should get aware of that very fast and in that case measures must be taken anyway. Do we really have to limit us so badly?

Copy link
Member

Choose a reason for hiding this comment

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

@BridgeAR AIUI this applies to changes that are not addressed because the reviewer wasn't clear about what they wanted.

The situations you're talking about sound like they're when the review has been addressed (either the change implemented, or a justification for not implementing provided). In that case if the reviewer doesn't respond that review can be considered addressed anyway, and shouldn't block landing.

Copy link
Member

Choose a reason for hiding this comment

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

@BridgeAR I don't have a problem with the approach you describe. Although part of me does want to see those things get escalated to TSC because it might help surface inactive Collaborators who maybe shouldn't be Collaborators anymore. Our processes, coding conventions, and underlying philosophy all evolve over time. If someone is not really paying attention for two years or something, it's probably time to remove them. (If they want to get involved again, cool, but they should go through an onboarding.)

Copy link
Member

@Trott Trott Sep 9, 2017

Choose a reason for hiding this comment

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

@BridgeAR Maybe here or in a subsequent PR, you (or I or someone else) can offer up text to clarify that it's OK to dismiss another Collaborator's request if:

  • it is believed in good faith that the request has been sufficiently addressed

AND

  • attempts to engage the Collaborator in conversation about it have gone unanswered.

(There may already be text somewhere to that effect? I'm not sure.)

Copy link
Member

Choose a reason for hiding this comment

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

I like that suggestion @Trott

I also see your point about escalating things to the TSC and we could maybe even have that in addition? So in case a review is dismissed because of one of the mentioned reasons, the TSC should also be notified about it? Even though it might at times also just be that someone is to involved and just does not handle all the notifications?

Copy link
Member

Choose a reason for hiding this comment

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

Even though it might at times also just be that someone is to involved and just does not handle all the notifications?

Sure, but we'll know the difference between "this person is active in the repo but clearly just didn't get around to this issue for whatever reason" vs. "oh, yeah, haven't seen that person around for a year and a half".

jasnell added a commit that referenced this pull request Sep 12, 2017
Explicitly allow vague objections to change requests to
be dismissed if requests for clarification go unanswered

PR-URL: #15233
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Ryan Graham <r.m.graham@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Sep 12, 2017

Landed in 2510500

@jasnell jasnell closed this Sep 12, 2017
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
Explicitly allow vague objections to change requests to
be dismissed if requests for clarification go unanswered

PR-URL: nodejs#15233
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Ryan Graham <r.m.graham@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
jasnell added a commit that referenced this pull request Sep 20, 2017
Explicitly allow vague objections to change requests to
be dismissed if requests for clarification go unanswered

PR-URL: #15233
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Ryan Graham <r.m.graham@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@MylesBorins
Copy link
Contributor

v6.x?

@MylesBorins
Copy link
Contributor

ping @jasnell

MylesBorins pushed a commit that referenced this pull request Dec 19, 2017
Explicitly allow vague objections to change requests to
be dismissed if requests for clarification go unanswered

PR-URL: #15233
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Ryan Graham <r.m.graham@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 20, 2017
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.