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

Merge some lints together #1078

Closed
mcarton opened this issue Jul 9, 2016 · 3 comments · Fixed by #5563
Closed

Merge some lints together #1078

mcarton opened this issue Jul 9, 2016 · 3 comments · Fixed by #5563
Labels
A-documentation Area: Adding or improving documentation C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages S-needs-discussion Status: Needs further discussion before merging or work can be started
Milestone

Comments

@mcarton
Copy link
Member

mcarton commented Jul 9, 2016

Clippy has a lot of lints now, but sometimes there is not much difference between two lints. I think we should merge some of them together.

Advantages

  • Slightly shorter big-list-of-all-the-lints.
  • Less documentation duplication (see new_without_default and new_without_default_derive).
  • Two lints should be different only when one would want to set different allow/warn/deny level for them. Why would anyone ever want to forbid calls to Option::unwrap but not Result::unwrap?
  • Less confusing as to why there are two different lints for that.

Disadvantages

  • Might be a breaking change, but IIRC rustc has a way to indicate a lint was renamed (and hopefully handle properly #[allow]s etc. on it).
  • Need to find new names.

Candidates

Here are some candidates (there might be other):

@mcarton mcarton added C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages S-needs-discussion Status: Needs further discussion before merging or work can be started A-documentation Area: Adding or improving documentation labels Jul 9, 2016
@Manishearth
Copy link
Member

Bit iffy on string_add, but otherwise LGTM.

Perhaps this should be tabled till the RfC at https://github.com/Manishearth/rust-clippy/issues/533? I'm thinking of starting the RfC this week or so, so add more issues to that thread if you find them.

@mcarton
Copy link
Member Author

mcarton commented Jul 9, 2016

Bit iffy on string_add, but otherwise LGTM.

There already is assign_op_pattern vs. assign_ops for the + vs. += difference (and those really are different, they should not be merged), so keeping the distinction between those just for the String case seems redundant IMO.

@Manishearth
Copy link
Member

I guess so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documentation Area: Adding or improving documentation C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages S-needs-discussion Status: Needs further discussion before merging or work can be started
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants