-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Fixed number after underscore in camel case #13109
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
fn invalid_following_underscore(ident: &str) -> bool { | ||
for i in range(0, ident.char_len() - 1) { | ||
if ident.char_at(i) != '_' { continue; } | ||
if !"_0123456789".contains_char(ident.char_at(i + 1)) { |
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.
This isn't doing what you expect: it's getting the character after the i + 1
th byte, where i
is the count of the current character (i.e. not it's byte index, and so will break on an multibyte codepoints) and similarly the char_at
above is wrong too. I think this loop could be written correctly as:
let last_was_underscore = false;
for c in ident.chars() {
if last_was_underscore && !"_0123456789".contains_char(c) {
return true;
}
last_was_underscore = c == '_';
}
false
(untested)
Also, this changes the behaviour to reject _A
, where as that was ok previously.
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.
You are correct. I had a working algorithm in the issue, but yours is better. I have an older incorrect version in this pull request.
I'm a little worried about adding exceptions to our stylistic lints. It's pretty easy to ignore them via an @bvssvni, do you know if there's precedent for this sort of exception? Googling around for camel case I didn't find anything related to this. Is there a reason that an |
The issue here is code generation. This case was discovered when I worked on documenting Cairo bindings https://github.com/jensnockert/cairo.rs/search?q=SVGVersion&ref=cmdform. The code is generated from XML files using a Ruby script. It requires a check in these particular places and inject the attribute if one wants to keep the library idiomatic Rust elsewhere. |
I've found that bindings to other systems which do not necessarily repsect Rust's style often must opt-out of the stylistic lints. For example, all bindings to windows functions have to opt-out of the lints because they use camel case for variables and often uppercase variables. Additionally, LLVM has quite different style than rust does. I would imagine that an |
Yes, that would work. It is not a major problem for me. |
I did some searching and could not find any precedence for this. The tools that convert from underscore to camel case either removes numbers or merges them together, which means there is no conventions to write multiple numbers in this style. It is easier to add those exceptions later than removing them. I talked to somebody on the rust-internals IRC channel. One was ok with it (if precedence) and one was against. Nobody was strongly in favor of it, neither am I. Closing. |
Closes #12986