Add lint for variables whose names contain uppercase characters #12300
Conversation
|
This is an RFC at the moment. Its not currently warning on uppercase variables in function parameters and there may be other missing locations as well. I also haven't completed a make check yet. If this looks like a good addition, I'll add warnings for whatever places I've missed. |
|
👍 I think there's an issue filed somewhere to add this lint. |
|
Maybe we could have it lint for non-snake-case variables? i.e. check for uppercase letters anywhere in the identifier. |
|
I've updated the PR to warn on any uppercase letter in a variable name and to also warn on function parameters. |
|
I realized that I'm not warning on struct fields which it would probably make sense to warn on as well. |
|
I just pushed changes to also warn for struct fields with uppercase characters in their names. |
|
Looks good! Although I think having "uppercase variables" cover uppercase struct fields too is slightly peculiar (rather than being a separate lint... but then having a lint just for that might be even weirder). |
|
@huonw I agree its a bit weird. My thinking is that I can't come up with a scenario where someone would want to enable one without the other. I'm happy to split them apart, though. I also noticed that I'm not current catching variable names in extern functions. I don't know if it makes sense to try to catch those or not. I also noticed that there were some variables that need renaming that I missed, so, I'm working on getting that all cleaned up |
|
I rebased the PR again and now its passing make check NO_BENCH=1, at least on my computer. Unless there are other things that I should do (split it into two lints for variables and struct fields; catch variables in extern functions; or something else), I think this is mergeable if it looks good. |
|
@DaGenix thanks for this. The PR needs to be rebased again. |
|
Cool. I just rebased. |
|
r=me with a slight adjustment to the error message on struct fields. |
|
Rebased to change the warning message on struct fields. |
|
In light of these errors, perhaps only the first character of variables/structure fields should be linted for now? |
|
I'm fine with making this change - this was what the original version of my patch did. However, these all look like windows functions which naturally don't conform to the Rust standards. How about disabling the lint in this module? |
|
I think this is specific to windows for now, but we should probably start off as conservative before going all the way. While it may be just windows currently, it may be indicative of more unexpected code that purposefully doesn't follow this style. |
|
Rebased to only warn if the first character is uppercase. I left in my changes to rename everything I could find that had uppercase characters anywhere in their name since its my understanding that all lowercase variables names are the preferred style. |
|
It appears that I accidentally resurrected libstd/to_bytes.rs. I'll rebase this tomorrow to pull that out. Please don't merge until I get a chance to do that. Sorry. |
|
Rebased and got rid of to_bytes.rs. |
|
bah! Stupid Windows! |
… use only lowercase characters
…th uppercase characters
|
Rebased against master again. I added a commit to #[allow(uppercase_variables)] in libstd/libc.rs since functions in this module probably don't need to follow Rust variable naming conventions. I wasn't able to get a Windows build to work due to some compiler errors that I don't fully understand. I think this should work on Windows this time, though. |
…ichton
I added a new lint for variables whose names contain uppercase characters, since, by convention, variable names should be all lowercase. What motivated me to work on this was when I ran into something like the following:
```rust
use std::io::File;
use std::io::IoError;
fn main() {
let mut f = File::open(&Path::new("/something.txt"));
let mut buff = [0u8, ..16];
match f.read(buff) {
Ok(cnt) => println!("read this many bytes: {}", cnt),
Err(IoError{ kind: EndOfFile, .. }) => println!("Got end of file: {}", EndOfFile.to_str()),
}
}
```
I then got compile errors when I tried to add a wildcard match pattern at the end which I found very confusing since I believed that the 2nd match arm was only matching the EndOfFile condition. The problem is that I hadn't imported io::EndOfFile into the local scope. So, I thought that I was using EndOfFile as a sub-pattern, however, what I was actually doing was creating a new local variable named EndOfFile. This lint makes this error easier to spot by providing a warning that the variable name EndOfFile contains a uppercase characters which provides a nice hint as to why the code isn't doing what is intended.
The lint warns on local bindings as well:
```rust
let Hi = 0;
```
And also struct fields:
```rust
struct Something {
X: uint
}
```
I added a new lint for variables whose names contain uppercase characters, since, by convention, variable names should be all lowercase. What motivated me to work on this was when I ran into something like the following:
I then got compile errors when I tried to add a wildcard match pattern at the end which I found very confusing since I believed that the 2nd match arm was only matching the EndOfFile condition. The problem is that I hadn't imported io::EndOfFile into the local scope. So, I thought that I was using EndOfFile as a sub-pattern, however, what I was actually doing was creating a new local variable named EndOfFile. This lint makes this error easier to spot by providing a warning that the variable name EndOfFile contains a uppercase characters which provides a nice hint as to why the code isn't doing what is intended.
The lint warns on local bindings as well:
And also struct fields: