-
Notifications
You must be signed in to change notification settings - Fork 1.8k
split clippy into lints, plugin and cargo-clippy #950
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
Conversation
@@ -0,0 +1,25 @@ | |||
[package] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this should be an rlib? This would make compilation fast again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why that would speed it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, wait, no, the default cargo behavior would work here anyway. Nvm
This is awesome! I was dreading having to make this change 😛 Thanks so much! Overall lgtm, I'll have a closer look tomorrow. |
Well... You're gonna have to publish two crates from now on instead of one, + keep the versions synced I think, I don't know if cargo allows |
Three crates, actually? |
Oh, only two. |
rebased |
This looks good to me. @mcarton @birkenfeld @llogiq Note that there are two package versions to bump now. |
@@ -30,6 +29,7 @@ semver = "0.2.1" | |||
toml = "0.1" | |||
unicode-normalization = "0.1" | |||
quine-mc_cluskey = "0.2.2" | |||
clippy_lints = { version = "0.0.*", path = "clippy_lints" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it occurred to me that we might want to hardcode the version so as to avoid upgrade pains -- people shouldn't have to cargo update -p clippy
and then still get errors because they didn't upgrade clippy_lints. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can add this to utils/update_lints.py so the version is automatically checked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sweet. separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe someone who knows python should do that... or I can port it to Rust
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcarton ?
Otherwise I'll fix it monday
an alternative to #933