-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add lint long_variable_names
#14818
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
base: master
Are you sure you want to change the base?
Add lint long_variable_names
#14818
Conversation
r? @llogiq thanks for your clippy workshop today! Looking forward for your feedback. |
I doubt whether the default value is well chosen. 30 seems ok for most of the use cases. But for the clippy source it was needed to create some different configurations too. |
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.
Yeah, so I'd like to make sure that we have some benefit if we actually have that lint warn. I figure we either make it a pedantic lint or we set a far higher default threshold. When in doubt, you can run lintcheck.
Otherwise this looks good to merge.
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.
Looks good to me. Can you squash your commits please?
c7c54a6
to
99a3a2e
Compare
I will start the FCP for this lint later today. |
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.
Thanks for incorporating the feedback. Here is another round of how to make things more efficient, with even simpler code.
format!( | ||
"use of a long variable name, it is longer than the configured `max-variable-name-length` of {} characters", | ||
self.max_variable_name_length | ||
), | ||
None, | ||
format!("reduce the length of the long variable name with at least {length_diff} characters"), |
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.
I'd prefer something like that, but it is mostly nitpicking:
format!( | |
"use of a long variable name, it is longer than the configured `max-variable-name-length` of {} characters", | |
self.max_variable_name_length | |
), | |
None, | |
format!("reduce the length of the long variable name with at least {length_diff} characters"), | |
format!( | |
"variable name is longer than the configured `max-variable-name-length` ({} characters)", | |
self.max_variable_name_length | |
), | |
None, | |
format!("rename the variable to not exceed the maximum length"), |
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.
I agree with the changes in line 65. However line 69 contains information that a developer can use to make decisions in what to do, for example to remove one word from the variable.
Updated your description to prevent fully closing #644, which I assume is undesired 😅 |
a087635
to
e34ef64
Compare
The lint is configurable by using `max-variable-name-length` and it is set to 100 by default.
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's much improved, thanks!
You're welcome! Thanks for the quick feedback loop, so we were able to finish it fast. It was a nice learning experience on how to build a first lint. |
The lint is configurable by using
max-variable-name-length
.Related #644 this PR only concerns the variable length
changelog: [
long_variable_names
]: enable variable length check in clippy