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

Factor out clippy_utils crate #6756

Merged
merged 18 commits into from
Feb 24, 2021
Merged

Factor out clippy_utils crate #6756

merged 18 commits into from
Feb 24, 2021

Conversation

smoelius
Copy link
Contributor

@smoelius smoelius commented Feb 17, 2021

As discussed in #6746, this PR factors out clippy_lints::utils as its own crate, clippy_utils .

This change will allow clippy_utils to be used in lints outside of Clippy.

There is no plan to publish this crate on crates.io (see #6746 (comment)). Dependent crates should obtain it from GitHub.

changelog: Factor out clippy_utils so it can be used by external tools (not published)

@rust-highfive
Copy link

r? @phansch

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 17, 2021
@Manishearth
Copy link
Member

(we should still squat the crate on crates.io, though)

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

+1 for squatting that crate name, but that isn't a blocker for this PR. And we should only publish a dummy like clippy_dummy to avoid confusion.

I also think there are some references to utils in the doc dir.

clippy_lints/src/lib.rs Outdated Show resolved Hide resolved
clippy_utils/Cargo.toml Show resolved Hide resolved
@@ -0,0 +1,574 @@
#![allow(clippy::float_cmp)]
Copy link
Member

Choose a reason for hiding this comment

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

Weird that git doesn't recognize that this file was just moved 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a small difference:

3c3
< use crate::utils::{clip, sext, unsext};
---
> use crate::{clip, sext, unsext};

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's fine, but git manged to figure this out for other files. That's what confuses me.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can tweak the git rename sensitivity (docs)

@smoelius
Copy link
Contributor Author

I also think there are some references to utils in the doc dir.

Thanks. I think I got them all: smoelius@3341e3d

@phansch
Copy link
Member

phansch commented Feb 18, 2021

r? @flip1995 (I'm short on time this week)

@rust-highfive rust-highfive assigned flip1995 and unassigned phansch Feb 18, 2021
@bors
Copy link
Collaborator

bors commented Feb 21, 2021

☔ The latest upstream changes (presumably #6754) made this pull request unmergeable. Please resolve the merge conflicts.

@flip1995
Copy link
Member

flip1995 commented Feb 21, 2021

This needs a rebase.

@bors
Copy link
Collaborator

bors commented Feb 23, 2021

☔ The latest upstream changes (presumably #6573) made this pull request unmergeable. Please resolve the merge conflicts.

@smoelius
Copy link
Contributor Author

I rebased this again, following #6573. Please let me know if there is anything I can do to make the review go more smoothly.

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

LGTM. But I want a second approval on this, since it is a bigger change @rust-lang/clippy

@flip1995
Copy link
Member

flip1995 commented Feb 24, 2021

@bors r=flip1995,oli-obk

EDIT: Accidentally shipped oli-obk with obi-wan kenobi.

@bors
Copy link
Collaborator

bors commented Feb 24, 2021

📌 Commit ab7381f has been approved by flip1995,obi-obk

@bors
Copy link
Collaborator

bors commented Feb 24, 2021

⌛ Testing commit ab7381f with merge 489c4f0...

@bors
Copy link
Collaborator

bors commented Feb 24, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995,obi-obk
Pushing 489c4f0 to master...

@bors bors merged commit 489c4f0 into rust-lang:master Feb 24, 2021
@matthiaskrgr
Copy link
Member

Mmh, looks like this broke some part of CI ..?
https://github.com/rust-lang/rust-clippy/runs/1968986956#step:7:494

@matthiaskrgr
Copy link
Member

Fix attempt at #6785

@smoelius
Copy link
Contributor Author

Thank you @matthiaskrgr. An alternative solution (I think) is #6786.

@flip1995 argued that declare_clippy_lint should not be made public. I suppose the same argument could be made that Conf should not be made public. I don't have strong feelings either way.

@smoelius
Copy link
Contributor Author

Actually, because there is more to conf.rs than just Conf, I think I prefer #6785. But I could split up conf.rs and amend #6786, if preferred.

@flip1995
Copy link
Member

I prefer the solution by @smoelius. I don't think publishing conf.rs helps anyone. It is really specific to Clippy.

bors added a commit that referenced this pull request Feb 24, 2021
…=flip1995

Move conf.rs back into clippy_lints

This is an alternative solution to #6785 to fix the CI break caused by #6756.
bors added a commit that referenced this pull request Feb 24, 2021
…=flip1995

Move conf.rs back into clippy_lints

This is an alternative solution to #6785 to fix the CI break caused by #6756.

changelog: none
bors added a commit that referenced this pull request Feb 24, 2021
…=flip1995

Move conf.rs back into clippy_lints

This is an alternative solution to #6785 to fix the CI break caused by #6756.

changelog: none
@smoelius
Copy link
Contributor Author

Thank you, very sincerely, @oli-obk, @Manishearth, @matthiaskrgr, and especially @flip1995.

@smoelius smoelius deleted the clippy_utils branch March 1, 2022 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants