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

Add new lint negative_feature_names and redundant_feature_names #7539

Merged
merged 1 commit into from
Aug 23, 2021
Merged

Add new lint negative_feature_names and redundant_feature_names #7539

merged 1 commit into from
Aug 23, 2021

Conversation

Labelray
Copy link
Contributor

@Labelray Labelray commented Aug 6, 2021

Add new lint [negative_feature_names] to detect feature names with prefixes no- or not- and new lint [redundant_feature_names] to detect feature names with prefixes use-, with- or suffix -support
changelog: Add new lint [negative_feature_names] and [redundant_feature_names]

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @camsteffen (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 6, 2021
@camsteffen
Copy link
Contributor

There are two separate issues being linted here:

  • Features beginning with no- or not- - features should be additive, the name suggests otherwise
  • Features beginning with use- or with- - needless prefix

I think this should be two separate lints - negative_feature_name and feature_name_verbosity (feel free to offer other ideas). I don't think "confusing" is the right word in either case. The lint output should clearly explain what is wrong with the name.

Otherwise, this is nice, thanks!

Fixes #1746

@kangalio
Copy link

kangalio commented Aug 6, 2021

Could -support postfix be detected too? For example

[features]
embedded-graphics-support = ["embedded-graphics"]
serde-support = ["serde"]
tokio-support = ["tokio"]

Here's plenty of examples of this in the wild: https://grep.app/search?q=-support%20%3D%20%5B&filter[path.pattern][0]=Cargo.toml&filter[lang][0]=TOML

@Labelray
Copy link
Contributor Author

Labelray commented Aug 7, 2021

Good advices! I am going to close this PR and implement two separate lints:

  • [feature_name_redundancy] detecting prefix with-, use- and postfix support
  • [negative_feature_name] detecting prefix not-, no-

@Labelray Labelray closed this Aug 7, 2021
@camsteffen
Copy link
Contributor

You could, if you want, reopen and edit this PR. The two lints can have a shared implementation, just with two declare_clippy_lint!.

@Labelray Labelray reopened this Aug 7, 2021
@Labelray
Copy link
Contributor Author

Labelray commented Aug 9, 2021

Lints [negative_feature_name] and [redundant_feature_name] done

clippy_lints/src/feature_name.rs Outdated Show resolved Hide resolved
clippy_lints/src/feature_name.rs Outdated Show resolved Hide resolved
clippy_lints/src/feature_name.rs Outdated Show resolved Hide resolved
clippy_lints/src/feature_name.rs Outdated Show resolved Hide resolved
clippy_lints/src/feature_name.rs Outdated Show resolved Hide resolved
clippy_lints/src/feature_name.rs Show resolved Hide resolved
clippy_lints/src/feature_name.rs Outdated Show resolved Hide resolved
clippy_lints/src/feature_name.rs Show resolved Hide resolved
clippy_lints/src/feature_name.rs Outdated Show resolved Hide resolved
clippy_lints/src/feature_name.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@camsteffen camsteffen left a comment

Choose a reason for hiding this comment

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

Empty stderr files should be removed.

clippy_lints/src/feature_name.rs Outdated Show resolved Hide resolved
clippy_lints/src/feature_name.rs Outdated Show resolved Hide resolved
clippy_lints/src/feature_name.rs Outdated Show resolved Hide resolved
clippy_lints/src/feature_name.rs Outdated Show resolved Hide resolved
clippy_lints/src/feature_name.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@camsteffen camsteffen left a comment

Choose a reason for hiding this comment

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

There are still unresolved comments from previous reviews.

clippy_lints/src/feature_name.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@camsteffen camsteffen left a comment

Choose a reason for hiding this comment

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

There is still an empty stderr file to be deleted.

clippy_lints/src/feature_name.rs Outdated Show resolved Hide resolved
clippy_lints/src/feature_name.rs Outdated Show resolved Hide resolved
clippy_lints/src/feature_name.rs Outdated Show resolved Hide resolved
clippy_lints/src/feature_name.rs Outdated Show resolved Hide resolved
clippy_lints/src/feature_name.rs Outdated Show resolved Hide resolved
clippy_lints/src/feature_name.rs Show resolved Hide resolved
Copy link
Contributor

@camsteffen camsteffen left a comment

Choose a reason for hiding this comment

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

Getting close. Thanks for your patience.

clippy_lints/src/feature_name.rs Outdated Show resolved Hide resolved
clippy_lints/src/feature_name.rs Outdated Show resolved Hide resolved
clippy_lints/src/feature_name.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@camsteffen camsteffen left a comment

Choose a reason for hiding this comment

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

Good to go! Please squash commits.

@camsteffen camsteffen added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Aug 20, 2021
@camsteffen
Copy link
Contributor

Can you squash the first commit as well since we are not keeping the initial lint name?

@camsteffen
Copy link
Contributor

Also please update the PR title and changelog in first post.

@Labelray Labelray changed the title Add new lint confusing_features_naming Add new lint negative_feature_names and redundant_feature_names Aug 21, 2021
@camsteffen
Copy link
Contributor

Thanks! @bors r+

@bors
Copy link
Contributor

bors commented Aug 23, 2021

📌 Commit 0a021d5 has been approved by camsteffen

@bors
Copy link
Contributor

bors commented Aug 23, 2021

⌛ Testing commit 0a021d5 with merge 22606e7...

@bors
Copy link
Contributor

bors commented Aug 23, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: camsteffen
Pushing 22606e7 to master...

@bors bors merged commit 22606e7 into rust-lang:master Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants