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

lint audit: Implementation + move one lint #2579

Merged
merged 5 commits into from Mar 29, 2018
Merged

lint audit: Implementation + move one lint #2579

merged 5 commits into from Mar 29, 2018

Conversation

@oli-obk
Copy link
Collaborator

@oli-obk oli-obk commented Mar 27, 2018

I still need to figure out how to make warn(clippy) cause warn(clippy_style, clippy_perf, ...)

oli-obk added 3 commits Mar 27, 2018
Copy link
Member

@Manishearth Manishearth left a comment

LGTM. There are a bunch of cases where I think another category may also apply; but it's up to you if we should choose that instead. Unsure which.

pub DOUBLE_PARENS, Warn,
declare_clippy_lint! {
pub DOUBLE_PARENS,
complexity,

This comment has been minimized.

@Manishearth

Manishearth Mar 29, 2018
Member

perhaps style? idk. complexity makes sense too

This comment has been minimized.

@oli-obk

oli-obk Mar 29, 2018
Author Collaborator

My idea of complexity is anything that makes the code look more complex than it is, and everyone can agree that it should be improved. It's a fuzzy boundary, but I want to keep complexity lints as lints that no matter of personal style references, everyone wants to make those changes.

This comment has been minimized.

@Manishearth

Manishearth Mar 29, 2018
Member

ah, that's a reasonable boundary, makes sense

@@ -22,9 +22,9 @@ use utils::{camel_case_from, camel_case_until, in_macro};
/// HummingbirdCake,
/// }
/// ```
declare_lint! {
declare_clippy_lint! {

This comment has been minimized.

@Manishearth

Manishearth Mar 29, 2018
Member

While we're here, we should document it such that the difference between the two is obvious

This comment has been minimized.

@oli-obk

oli-obk Mar 29, 2018
Author Collaborator

done

pub FALLIBLE_IMPL_FROM, Allow,
declare_clippy_lint! {
pub FALLIBLE_IMPL_FROM,
nursery,

This comment has been minimized.

@Manishearth

Manishearth Mar 29, 2018
Member

should this still be nursery?

(did TryFrom stabilize?)

This comment has been minimized.

@oli-obk

oli-obk Mar 29, 2018
Author Collaborator

I don't think it stabilized

This comment has been minimized.

@Manishearth

Manishearth Mar 29, 2018
Member

belongs in the nursery then, yes? It's allow because we don't want to make it warn yet, not because we don't want to make it warn ever

pub POSSIBLE_MISSING_COMMA,
Warn,
style,

This comment has been minimized.

@Manishearth

Manishearth Mar 29, 2018
Member

correctness?

pub TOO_MANY_ARGUMENTS,
Warn,
style,

This comment has been minimized.

@Manishearth

Manishearth Mar 29, 2018
Member

perhaps also complexity? idk

This comment has been minimized.

@oli-obk

oli-obk Mar 29, 2018
Author Collaborator

neither do I, I'll put it there, if ppl complain we can put it back.

pub BOOL_COMPARISON,
Warn,
style,

This comment has been minimized.

@Manishearth

Manishearth Mar 29, 2018
Member

complexity?

pub PRINTLN_EMPTY_STRING,
Warn,
style,

This comment has been minimized.

@Manishearth

Manishearth Mar 29, 2018
Member

the lint description here is incorrect

pub BOX_VEC,
Warn,
complexity,

This comment has been minimized.

This comment has been minimized.

@oli-obk

oli-obk Mar 29, 2018
Author Collaborator

both ^^ it's not just slower or requires more memory, it's just weird

pub LINT_AUTHOR,
Warn,
style, // ok, this is not a style lint, but it's also a noop without the appropriate attribute

This comment has been minimized.

@Manishearth

Manishearth Mar 29, 2018
Member

Can we keep it in the nursery or internal instead?

pub DEEP_CODE_INSPECTION,
Warn,
style, // not a style lint, but essentially a noop without the appropriate attribute

This comment has been minimized.

@Manishearth

Manishearth Mar 29, 2018
Member

ditto wrt nursery

oli-obk added 2 commits Mar 29, 2018
@oli-obk oli-obk merged commit a47734c into master Mar 29, 2018
4 checks passed
4 checks passed
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@oli-obk oli-obk deleted the lint_audit_mcve branch Mar 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants