-
Notifications
You must be signed in to change notification settings - Fork 1.7k
validate lint name in clippy_dev
#10817
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
r? @dswij (rustbot has picked a reviewer for you, use r? to override) |
clippy_dev/src/main.rs
Outdated
.required(true) | ||
.value_parser(|name: &str| { | ||
if name.contains('-') { | ||
Err("Lint name cannot contain `-`, use `_` instead.") |
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.
Instead of error, would it make sense to just replace -
with _
? I don't think there ever is a use case where you want -
.
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.
That makes sense to me (assuming that was directed at me). I think it's safe to assume that the user probably didn't want the literal character -
but just meant to have it as a delimiter and replacing it with _
is fine
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.
Yes, that question was for you :)
Co-authored-by: Philipp Krones <[email protected]>
@bors r+ Thanks! |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
This PR adds a little bit of validation to
cargo dev new_lint
. I've had it happen a few times where I wanted to add a new lint, but forgot that lint names cannot contain-
. If you try to do it anyway,clippy_dev
will generate illegal syntax (like addingmod test-lint;
to clippy_lints/src/lib.rs for the module declaration). Maybe having it error out early would be helpful to others too.changelog: none