-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 upper_case_acronyms
#6475
Conversation
r? @flip1995 (rust-highfive has picked a reviewer for you, use r? to override) |
Also, I think this might have to be pedantic, as it will generate excessive warnings in some projects. |
I have the same spontaneous feeling on two-letter acronyms but IMO they aren't exceptions in Rust given IoSlice, TypeId and GlObject.
If two-letter acronyms are exceptions pedantic is fair as the lint will be false-positive (including ID and IO) prone. If not I'd prefer this lint to be deny-by-default since the lint should be as broad as possible, considering the naming convention on acronyms are not intuitive for those who came from other languages. |
The I'll let the reviewer decide on whether this should be pedantic or something else. I tested it on a bunch of crates and there weren't that many warnings. The one issue I found is that it lints on globals created with lazy_static. use lazy_static::lazy_static;
lazy_static! {
/// This is an example for using doc comment attributes
static ref EXAMPLE: u8 = 42;
}
fn main() {}
There probably needs to be a macro check somewhere. I was also wondering if we should call the lint |
Got it. I guess I need opinions from others on its lint category.
Very nice catch! I'll make the changes next week as I have a lot to do right now. |
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 think style is appropriate for this lint.
I tried to find some Rust documentation on capitalization conventions of acronyms but I didn't find anything.
@mikerite The API guidelines of rust mention this in the paragraph after the table, you can find here: https://rust-lang.github.io/api-guidelines/naming.html#casing-conforms-to-rfc-430-c-case
@matsujika ^ Please link this in the link documentation instead, since it is more current.
68c9bbd
to
97ecc06
Compare
This comment has been minimized.
This comment has been minimized.
capitalized_acronyms
upper_case_acronyms
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.
Two small things left
/// ```rust | ||
/// struct HttpResponse; | ||
/// ``` | ||
pub UPPER_CASE_ACRONYMS, |
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.
pub UPPER_CASE_ACRONYMS, | |
pub UPPERCASE_ACRONYMS, |
Uppercase is usually written as one word. Like in the char::to_uppercase
method
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.
There's non-upper-case-globals
lint in rustc and I suppose it's reasonable to make the name of this lint similar to that. (It seems they wanted to go consistent with non-camel-case-types
and non_snake_case
when naming the rustc lint)
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.
Well, I find that inconsistent. But lets be consistently inconsistent and keep the name as it is 👍
d14c073
to
70a8631
Compare
This comment has been minimized.
This comment has been minimized.
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! Please squash some of your commits, so we can merge this.
70a8631
to
ab1da8f
Compare
Co-authored-by: Philipp Krones <[email protected]>
@bors r+ Thanks! |
📌 Commit 0ccb491 has been approved by |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Close #1335
I need some reviews on the English sentences because I feel they're messed up. ;)
changelog: Add new lint
upper_case_acronyms