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

Misc changes to clippy_config #13173

Merged
merged 6 commits into from
Jul 29, 2024
Merged

Misc changes to clippy_config #13173

merged 6 commits into from
Jul 29, 2024

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Jul 28, 2024

Contains part of #13084

Changes include:

  • Sort config list and each configs lint list.
  • Add default text for the two configs that were missing it.
  • Switch the lint list in the configs to an attribute.
  • Make dev fmt sort the config list.

r? @xFrednet

changelog: none

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 28, 2024
Comment on lines +126 to +127
$(#[lints($($for_lints:ident),* $(,)?)])?
$name:ident: $ty:ty = $default:expr,
Copy link
Member

Choose a reason for hiding this comment

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

This is so nice! Thanks!

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

The changes to the macro look more than good to me. I really like the changes to declare the lints via attributes.

I'm not quite sure about the cargo dev fmt changes. It's cool, that it actually formats the whole thing and not just errors like rustc's tidy does. The downside is, that it feels like the custom format adds a lot of complexity. Future changes to the Conf! macro will likely also require some adaption of the formatting code.

I can't make up my mind about this rn. If you think that this is better than reusing tidy or rolling a simpler error on non alphabet check, I'd trust you.

clippy_dev/src/fmt.rs Show resolved Hide resolved
clippy_dev/src/fmt.rs Show resolved Hide resolved
clippy_dev/src/fmt.rs Show resolved Hide resolved
@xFrednet
Copy link
Member

Okay, I'm gonna R+ this now. Usually, I wouldn't have mentioned the sorted -> unstable_sorted nit since it's very nitpicky with such a change. But I thought it was mention worthy in case another change would have been needed.


roses are Red,
Biolets are Vlue,
Uan you Cnderstand,
this weird writing Too?

@bors
Copy link
Collaborator

bors commented Jul 29, 2024

📌 Commit 78a750e has been approved by xFrednet

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jul 29, 2024

⌛ Testing commit 78a750e with merge 3b64ca9...

@bors
Copy link
Collaborator

bors commented Jul 29, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 3b64ca9 to master...

@bors bors merged commit 3b64ca9 into rust-lang:master Jul 29, 2024
11 checks passed
@xFrednet
Copy link
Member

@rust-lang/clippy Small FYI: This PR changed the syntax of the define_Conf! macro.

The configs now roughly look like this:

/// Which crates to allow absolute paths from
#[lints(absolute_paths)]
absolute_paths_allowed_crates: FxHashSet<String> = FxHashSet::default(),

I like this change. Thank you to @Jarcho for this change :D

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.

5 participants