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 for multiple versions of dependencies #2672

Merged
merged 1 commit into from
May 1, 2018

Conversation

ysimonson
Copy link
Contributor

@ysimonson ysimonson commented Apr 16, 2018

Addresses #2665. Implementation notes:

  • I put this in the nursery as I'm not sure if in practice this'll end up being noisy. This is now in a new lint group for cargo-related lints: clippy_cargo.
  • Tests are failing for me for both this branch and master (rustc 1.27.0-nightly (bd40cbbe1 2018-04-14 / macos.)
  • Manually tested against both crates that do not have this issue (e.g. clippy itself) and crates that do have this issue (e.g. https://github.com/sharkdp/fd)

@ysimonson
Copy link
Contributor Author

@phansch thoughts on keeping this in nursery vs a new clippy_cargo group?

@phansch
Copy link
Member

phansch commented Apr 17, 2018

It's more of a categorization thing, because the cargo lints affect a tool and not rust code.
A clippy_cargo group would also allow non-cargo projects to enable or disable the cargo lints all at once. (Although we should probably encourage the use of cargo)

declare_clippy_lint! {
pub MULTIPLE_CRATE_VERSIONS,
nursery,
"Warn on multiple versions of the same crate being used"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for the "Warn on" here. It should just say what's wrong directly (in all lowercase, as tools are using that text)

@ysimonson
Copy link
Contributor Author

ysimonson commented Apr 17, 2018

OK, I put it in a clippy_cargo lint group. It looks like it's disabled by default anyways.

@phansch
Copy link
Member

phansch commented Apr 21, 2018

lgtm!

@oli-obk Would you like to have a look as well?

/// ```
declare_clippy_lint! {
pub MULTIPLE_CRATE_VERSIONS,
nursery,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should create a new lint group cargo in the declare_clippy_lint macro and use that instead of nursery. Otherwise the lint update Python script will move the lint back to the nursery

@oli-obk
Copy link
Contributor

oli-obk commented Apr 23, 2018

And now we need to update the python script :/

https://github.com/rust-lang-nursery/rust-clippy/blob/a47734c41d5ebc73bf0b457cf8f9075841132c28/util/update_lints.py#L126

Could you also add a mention of the new lint group to the README?

@oli-obk
Copy link
Contributor

oli-obk commented Apr 27, 2018

We'll need to figure out a way to test these lints someday...

@oli-obk
Copy link
Contributor

oli-obk commented Apr 27, 2018

Lgtm now. Please squash the commits into one. Then it's good to go

@ysimonson
Copy link
Contributor Author

Sorry for the dumb question, but is there a straight-forward way to collapse this to one commit, given I merged from master mid-way through to resolve conflicts? The only ways I can think of is to either squash merge (which I wouldn't be able to do) or open a new PR.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 29, 2018

You can rebase over the master branch. That'll kill all the merge commits. If you use git rebase -i origin/master you can squash commits at the same time

@oli-obk oli-obk merged commit f69dd6a into rust-lang:master May 1, 2018
@oli-obk
Copy link
Contributor

oli-obk commented May 1, 2018

Ok, now let's advertise this awesome lint. iirc there were a lot of ppl asking for it in different places, like cargo issues and irc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants