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

Loosen FCP restrictions #188

Merged
merged 2 commits into from
Feb 16, 2018
Merged

Loosen FCP restrictions #188

merged 2 commits into from
Feb 16, 2018

Conversation

aturon
Copy link
Member

@aturon aturon commented Feb 16, 2018

Allows an item to enter FCP when a majority of team members has approved
and no team member has objected. This retains the consensus process,
but makes it so that FCP isn't blocked by default. Rather, members who
object to the proposal need to register a concern to block it.

@aturon
Copy link
Member Author

aturon commented Feb 16, 2018

cc @SimonSapin @nikomatsakis

@nikomatsakis
Copy link
Contributor

Hmm, I'm torn. I agree that the current model is annoying. I'm not sure that majority is sufficient "quorum". I know that I often don't register an objection but also don't click because I want to think -- probably it's good to force me to articulate my concerns though. So 👍 on the idea, probably 👍 on the specifics.

@nikomatsakis
Copy link
Contributor

I just realized that all this does is ENTER the FCP period. I was concerned about things going "too fast", but that 10 day period is enough time. I say :shipit: !

@aturon
Copy link
Member Author

aturon commented Feb 16, 2018

@anp Please hold off on merging until I've heard from the rest of the core team on this -- I'll let you know,

@@ -224,6 +224,7 @@ fn evaluate_nags() -> DashResult<()> {
};

let num_active_reviews = reviews.iter().filter(|&&(_, ref r)| !r.reviewed).count();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be renamed to num_outstanding_reviews or num_pending_reviews to contrast it with complete reviews?

@@ -255,7 +256,7 @@ fn evaluate_nags() -> DashResult<()> {
};
}

if num_active_reviews == 0 && num_active_concerns == 0 {
if (num_active_reviews < num_complete_reviews) && num_active_concerns == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

We should obviously do whatever works best for the core team, but there are other options here than "all reviewed" or "majority reviewed" like 2/3, all but 1 (to account for people being away), etc.

Allows an item to enter FCP when a majority of team members has approved
and *no* team member has objected. This retains the consensus process,
but makes it so that FCP isn't blocked *by default*. Rather, members who
object to the proposal need to register a concern to block it.
@aturon
Copy link
Member Author

aturon commented Feb 16, 2018

Updated per @anp's review, and with the logic that there must be less than 3 reviews outstanding (and a majority must have approved).

I think we're good to go on this!

@anp anp merged commit 44e453e into rust-lang:master Feb 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants