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

upper_case_acronyms: move lint from style to pedantic lint group #6775

Merged
merged 1 commit into from
Feb 22, 2021

Conversation

matthiaskrgr
Copy link
Member

The lint does point out inconsistency with the Rust naming convention,
but the fact that rustc does not warn about the inconsistency by default
means that clippy probably should not warn by default either.

changelog: move upper_case_acronyms lint from style to pedantic group.

The lint does point out inconsistency with the Rust naming convention,
but the fact that rustc does not warn about the inconsistency by default
means that clippy probably should not warn by default either.

changelog: move upper_case_acronyms lint from style to pedantic group.
@rust-highfive
Copy link

r? @flip1995

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 22, 2021
@flip1995
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Feb 22, 2021

📌 Commit 0eefa61 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Feb 22, 2021

⌛ Testing commit 0eefa61 with merge 697f3b6...

@bors
Copy link
Contributor

bors commented Feb 22, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 697f3b6 to master...

@bors bors merged commit 697f3b6 into rust-lang:master Feb 22, 2021
@XAMPPRocky
Copy link
Member

the fact that rustc does not warn about the inconsistency by default
means that clippy probably should not warn by default either.

FWIW we just ran into some code that would be caught by this lint at Embark, and were surprised that this isn't a default warning by rustc. I think I would expect the opposite, that this lint warns by default and even be uplifted from clippy to rustc, since this naming convention for enum variants is actually part of RFC 430, and I would expect rustc to warn when things mismatch that RFC.

@matthiaskrgr
Copy link
Member Author

Hmm.
In the rustc repo for example, the lint triggers almost 200 times.
Some examples: RWLock EOFWhileParsingObject ABI ELFv1 CFGuard TargetCPUs LTO IntEQ IntNE PGO LLVMRustResult MD5 RustdocGUI.

EofWhileParsingObject, Elfv1 IntNe LlvmRustResult read kind of weird and are not an improvement besides adhering to some arbitrary guideline in my opinion.

I'm open to a revert of my pr but perhaps we can discuss this in the upcoming clippy meeting tomorrow.

@matthiaskrgr
Copy link
Member Author

@XAMPPRocky do you have some bugs that the lint found something like that?

@matthiaskrgr
Copy link
Member Author

There are numerous projects that have allow(clippy::upper_case_acronyms) already:
https://github.com/search?p=1&q=%22clippy%3A%3Aupper_case_acronyms%22&type=Code

@XAMPPRocky
Copy link
Member

XAMPPRocky commented Feb 23, 2021

The case we had looked something like this.

enum Direction {
    WEST,
    NORTH,
    EAST,
    SOUTH,
}

EofWhileParsingObject, Elfv1, IntNe, LlvmRustResult read kind of weird and are not an improvement besides adhering to some arbitrary guideline in my opinion.

I mean I think it’s no less arbitrary than non_camel_case_types, which rustc warns about by default. Though maybe if this was put in today it should be more conservative and just catch all uppercase or snake_case variants. To avoid setting off a lot of warnings.

@matthiaskrgr
Copy link
Member Author

I see, I think it makes sense to still warn for fully upper-case names.
I quickly threw together a POC at master...matthiaskrgr:upper_case_acr
This will issues a warn-by-default warning for fully upper case idents that are longer than 2 characters (so FP is not linted but WEST is.

@matthiaskrgr
Copy link
Member Author

#6788 moves the lint back to the style group.
It will warn by default on fully uppercase names like WEST.
If you turn on more aggressive linting in clippys config file via upper-case-acronyms-aggressive: true, it will also warn on LLVMRustResult.

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