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

meta: allow vague objections to be dismissed #15233

Closed
wants to merge 2 commits into from
Closed

meta: allow vague objections to be dismissed #15233

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
340ad76
meta: allow vague objections to be dismissed
jasnell Sep 6, 2017
5c13b17
[Squash] Nits
jasnell Sep 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions COLLABORATOR_GUIDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,12 @@ All pull requests that modify executable code should be subjected to
continuous integration tests on the
[project CI server](https://ci.nodejs.org/).

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

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 •
edited
Loading

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


#### Useful CI Jobs

* [`node-test-pull-request`](https://ci.nodejs.org/job/node-test-pull-request/)
Expand Down