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

Bikesheds to be done when RfCing clippy #533

Open
Manishearth opened this Issue Jan 2, 2016 · 19 comments

Comments

Projects
None yet
8 participants
@Manishearth
Collaborator

Manishearth commented Jan 2, 2016

At some point I wish to RfC the clippy lints.

One part of this will be figuring out Allow/Warn defaults. It will also be bikeshedding various questionable choices clippy has made.

Feel free to list anything you find bikesheddable here:

  • match_ref_pats on if let (is if let &foo = bar okay?) ; #532
  • Glob enum imports: warn or allow? #581
  • Two-branch matches: warn or allow? #579
  • shadow_unrelated: warn or allow
  • lints for specific libraries (e.g. #595): yay or nay?
  • redundant closure: warn or allow? There is something to be said for the added clarity of knowing how many arguments there are.
@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Jan 7, 2016

Collaborator

See also: #541

Collaborator

Manishearth commented Jan 7, 2016

See also: #541

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Jan 25, 2016

Collaborator

Another thing is shadow_unrelated – I consider those programmer lazyness, but I found so many matches in the wild that I haven't found the courage to make it warn by default.

Collaborator

llogiq commented Jan 25, 2016

Another thing is shadow_unrelated – I consider those programmer lazyness, but I found so many matches in the wild that I haven't found the courage to make it warn by default.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Jan 25, 2016

Collaborator

😄 I'm 👍 for warning it now if you want, btw, but yeah, it's something that we should list here.

Collaborator

Manishearth commented Jan 25, 2016

😄 I'm 👍 for warning it now if you want, btw, but yeah, it's something that we should list here.

@nagisa

This comment has been minimized.

Show comment
Hide comment
@nagisa

nagisa Jan 28, 2016

Two-branch matches: warn or allow?

Obviously allow. That’s purely a programmer/style-preference thing.

Glob enum imports: warn or allow?

Also allow?

fn a() {
     use Enum::*;
     // do a lot with the variants.
}

is pretty common, though I’m not sure if this exact pattern is linted against.

nagisa commented Jan 28, 2016

Two-branch matches: warn or allow?

Obviously allow. That’s purely a programmer/style-preference thing.

Glob enum imports: warn or allow?

Also allow?

fn a() {
     use Enum::*;
     // do a lot with the variants.
}

is pretty common, though I’m not sure if this exact pattern is linted against.

@oli-obk

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk Feb 1, 2016

Collaborator

@nagisa only glob imports at the module level are linted against. The question is whether to also not lint on pub use Enum::*; since that also seems to be a common pattern for old-style enums.

Collaborator

oli-obk commented Feb 1, 2016

@nagisa only glob imports at the module level are linted against. The question is whether to also not lint on pub use Enum::*; since that also seems to be a common pattern for old-style enums.

@oli-obk

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk Feb 1, 2016

Collaborator

That’s purely a programmer/style-preference thing.

@nagisa I changed the lint to only trigger if the second arm is a block. In that case it's both more vertical space AND more indentation when using a match instead of an if let.

Collaborator

oli-obk commented Feb 1, 2016

That’s purely a programmer/style-preference thing.

@nagisa I changed the lint to only trigger if the second arm is a block. In that case it's both more vertical space AND more indentation when using a match instead of an if let.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Feb 1, 2016

Collaborator

I changed the lint to only trigger if the second arm is a block

This is still preference/style based.

Collaborator

Manishearth commented Feb 1, 2016

I changed the lint to only trigger if the second arm is a block

This is still preference/style based.

@leodasvacas

This comment has been minimized.

Show comment
Hide comment
@leodasvacas

leodasvacas Feb 8, 2016

Allow cyclomatic_complexity, there is no universal answer to how complex is too complex. Allow explicit_iter_loop, &x is more concise but x.iter() is more explicit, not obvious which is more readable.

leodasvacas commented Feb 8, 2016

Allow cyclomatic_complexity, there is no universal answer to how complex is too complex. Allow explicit_iter_loop, &x is more concise but x.iter() is more explicit, not obvious which is more readable.

@mcarton

This comment has been minimized.

Show comment
Hide comment
@mcarton

mcarton Feb 8, 2016

Collaborator

When I made a PR to fix all Clippy warnings in Cargo, they asked me to remove my commit about explicit_iter_loop because they preferred x.iter() over &x.

Collaborator

mcarton commented Feb 8, 2016

When I made a PR to fix all Clippy warnings in Cargo, they asked me to remove my commit about explicit_iter_loop because they preferred x.iter() over &x.

@leodasvacas

This comment has been minimized.

Show comment
Hide comment
@leodasvacas

leodasvacas Jun 19, 2016

toplevel_ref_arg should be allowed for let bindings, it's purely stylistic, let ref x = expr can be more readable than let x = &expr when expr is complex (for example a long match).

leodasvacas commented Jun 19, 2016

toplevel_ref_arg should be allowed for let bindings, it's purely stylistic, let ref x = expr can be more readable than let x = &expr when expr is complex (for example a long match).

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Jun 26, 2016

Collaborator

#1043 is bikesheddable

Collaborator

Manishearth commented Jun 26, 2016

#1043 is bikesheddable

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Jun 27, 2016

Collaborator

Regarding #1043, I defend that !x is not only shorter, but also more idiomatic. I remember x == false from my time as university tutor, often used by students who failed to understand boolean logic. Also ! is more easily grepped than == false (and leaves no questions as to spacing/line breaks/..)

Collaborator

llogiq commented Jun 27, 2016

Regarding #1043, I defend that !x is not only shorter, but also more idiomatic. I remember x == false from my time as university tutor, often used by students who failed to understand boolean logic. Also ! is more easily grepped than == false (and leaves no questions as to spacing/line breaks/..)

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth
Collaborator

Manishearth commented Jul 9, 2016

@porglezomp

This comment has been minimized.

Show comment
Hide comment
@porglezomp

porglezomp Jul 10, 2016

Things like #1043 is why I prefer Python's not keyword to !...

porglezomp commented Jul 10, 2016

Things like #1043 is why I prefer Python's not keyword to !...

@leodasvacas

This comment has been minimized.

Show comment
Hide comment
@leodasvacas

leodasvacas Jul 26, 2016

Undeprecate string_to_string. I think it is unidiomatic to use to_string to clone a String. clone is clearer in intent. Also there are many lints related to clone, using to_string may hide bad practices.

leodasvacas commented Jul 26, 2016

Undeprecate string_to_string. I think it is unidiomatic to use to_string to clone a String. clone is clearer in intent. Also there are many lints related to clone, using to_string may hide bad practices.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Jul 27, 2016

Collaborator

I'm not sure if we should undeprecate, but a new lint sounds bikesheddable.

Collaborator

Manishearth commented Jul 27, 2016

I'm not sure if we should undeprecate, but a new lint sounds bikesheddable.

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Aug 29, 2016

Collaborator

@alexcrichton prefers .iter()/.iter_mut() to &/&mut,see futures-rs PR#55. I'm a bit torn on this issue (and I'm usually plenty opinionated). Showing newbies that they can use anything that implements IntoIterator in a for loop seems a good idea, is shorter and certainly rustic. On the other hand, the functions are more discoverable, and don't introduce sigils, to which some developers seem to have an aversion.

Perhaps the lint should not warn, but merely suggest that it is possible to iterate over &x instead of x.iter() (E.g. just issue a cx.sess().span_note_without_error(..)). I suspect this would make the lint appear friendlier.

Collaborator

llogiq commented Aug 29, 2016

@alexcrichton prefers .iter()/.iter_mut() to &/&mut,see futures-rs PR#55. I'm a bit torn on this issue (and I'm usually plenty opinionated). Showing newbies that they can use anything that implements IntoIterator in a for loop seems a good idea, is shorter and certainly rustic. On the other hand, the functions are more discoverable, and don't introduce sigils, to which some developers seem to have an aversion.

Perhaps the lint should not warn, but merely suggest that it is possible to iterate over &x instead of x.iter() (E.g. just issue a cx.sess().span_note_without_error(..)). I suspect this would make the lint appear friendlier.

@killercup

This comment has been minimized.

Show comment
Hide comment
@killercup

killercup Aug 31, 2016

Contributor

Oh sorry, I didn't see this issue when I created #1192: I'd to make option_map_unwrap_or and option_map_unwrap_or_else allow by default.

Contributor

killercup commented Aug 31, 2016

Oh sorry, I didn't see this issue when I created #1192: I'd to make option_map_unwrap_or and option_map_unwrap_or_else allow by default.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Oct 24, 2016

Collaborator

Default allow/warn for option_map_unwrap_or (see #1192)?

cc @shepmaster

Collaborator

Manishearth commented Oct 24, 2016

Default allow/warn for option_map_unwrap_or (see #1192)?

cc @shepmaster

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment