Rename `overlapping_patterns` lint by Nadrieril · Pull Request #78242 · rust-lan...

Contributor

varkor commented on Oct 23

@rust-lang/lang: this renames overlapping_patterns to overlapping_range_endpoints to address the confusing description of the lint, as discussed in #65477. This probably requires an FCP, or a sign-off.

The implementation itself looks fine.

nikomatsakis commented on Oct 23

Nominating to discuss the name in the lang-team meeting.

bors commented on Oct 25

The latest upstream changes (presumably #78334) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

bors commented on Oct 29

The latest upstream changes (presumably #78430) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

bors commented on Nov 2

The latest upstream changes (presumably #75534) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

Nadrieril commented on Nov 19

From watching the triage meeting it seems the reason for renaming the lint wasn't super clear, so I thought I'd clarify.

The issue is that overlapping_patterns sounds like it should lint on any kind of patterns that overlap. But that's really not what it does. What it does is specifically check if two ranges have a common endpoint, which could likely be a mistake:

match x {
	0..=10 => {} // Oops, probably meant to write `0..10`
	10..=20 => {}
	_ => {}
}

Knowing only the name of the lint, one would expect it to lint may more things. Even knowing that it applies to ranges, one would expect it to lint for overlaps like the following, but it doesn't:

match x {
	0..10 => {}
	5..15 => {}
	_ => {}
}

So it was proposed to rename it to overlapping_range_endpoints so that the name is an accurate description of what it lints for.

scottmcm commented on Nov 19

Thanks for watching and commenting! I'd certainly forgotten what this was about.

sounds like it should lint on any kind of patterns that overlap. But that's really not what it does.

I think that's a great summary, and I agree with niko's statement that a better name that other people like is good by me. So

@rfcbot merge

rfcbot commented on Nov 19 •

Team member @scottmcm has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

nikomatsakis commented on Nov 19

Yes, thanks for clarifying! And glad to know people watch the recordings. =)

@rfcbot reviewed

bors commented 27 days ago

The latest upstream changes (presumably #79243) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

bors commented 22 days ago

The latest upstream changes (presumably #79284) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

bors commented 20 days ago

The latest upstream changes (presumably #79523) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

Nadrieril commented 20 days ago

The worst thing is that all those merge conflicts are self-inflicted x)

joshtriplett commented 18 days ago

Renaming the lint we have seems good, to make it more self-documenting and less confusing.

I'd also like to see an implementation of the lint people thought overlapping_patterns was, but that'd be separate.

Nadrieril commented 18 days ago

I'd also like to see an implementation of the lint people thought overlapping_patterns was, but that'd be separate.

@joshtriplett That's being discussed here: #65477

rfcbot commented 12 days ago

This is now entering its final comment period, as per the review above.

rfcbot commented 2 days ago

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.