-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Refactor is_case_difference function
#115258
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2709,23 +2709,48 @@ impl Style { | |||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /// Whether the original and suggested code are visually similar enough to warrant extra wording. | ||||||||||
| /// Determines if the original code and the suggested code have sufficient visual similarity | ||||||||||
| /// to include extra textual information or descriptions to highlight the similarities or | ||||||||||
| /// differences between them. | ||||||||||
| /// | ||||||||||
| /// # Returns | ||||||||||
| /// | ||||||||||
| /// Returns `true` if the original and suggested code are visually similar enough to warrant | ||||||||||
| /// extra wording, otherwise `false`. | ||||||||||
| pub fn is_case_difference(sm: &SourceMap, suggested: &str, sp: Span) -> bool { | ||||||||||
| // FIXME: this should probably be extended to also account for `FO0` → `FOO` and unicode. | ||||||||||
| // Retrieve code | ||||||||||
| let found = match sm.span_to_snippet(sp) { | ||||||||||
| Ok(snippet) => snippet, | ||||||||||
| Ok(snippet) => { | ||||||||||
| snippet | ||||||||||
| } | ||||||||||
| Err(e) => { | ||||||||||
| warn!(error = ?e, "Invalid span {:?}", sp); | ||||||||||
| return false; | ||||||||||
| } | ||||||||||
| }; | ||||||||||
| let ascii_confusables = &['c', 'f', 'i', 'k', 'o', 's', 'u', 'v', 'w', 'x', 'y', 'z']; | ||||||||||
| // All the chars that differ in capitalization are confusable (above): | ||||||||||
| let confusable = iter::zip(found.chars(), suggested.chars()) | ||||||||||
| .filter(|(f, s)| f != s) | ||||||||||
| .all(|(f, s)| (ascii_confusables.contains(&f) || ascii_confusables.contains(&s))); | ||||||||||
| confusable && found.to_lowercase() == suggested.to_lowercase() | ||||||||||
| // FIXME: We sometimes suggest the same thing we already have, which is a | ||||||||||
| // bug, but be defensive against that here. | ||||||||||
| && found != suggested | ||||||||||
|
|
||||||||||
| // ASCII confusable characters when capitalization. | ||||||||||
| const ASCII_CONFUSABLES: [char; 12] = ['c', 'f', 'i', 'k', 'o', 's', 'u', 'v', 'w', 'x', 'y', 'z']; | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using a const does not make a difference, actually. Consts are copied on access, so this is actually worse, if anything (as we now potentially spill two copies on the attach, instead of one). Not that it actually matters, given than this is the error path, but still. You should probably change the type to |
||||||||||
|
|
||||||||||
| // # CHECK 1 | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, do you think those titles are useful? To me they seem somewhat pointless, they don't really add information or make it easier to scan the code, I think?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the original code, the comment sounds like Should I make it more descriptive?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what you mean, I was only talking about the |
||||||||||
| // 'zip' function is used to iterate over two sequences in parallel | ||||||||||
| found.chars() | ||||||||||
| .zip(suggested.chars()) | ||||||||||
|
Comment on lines
+2737
to
+2739
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no need to explain Additionally I'd suggest using
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for the remainder :) Actually, I'm still new to Rust, and there's a lot I'm still catching on to. About the |
||||||||||
| // - Check if the characters are equal, OR | ||||||||||
| // - Check character `f` is present in the `ASCII_CONFUSABLES` array, OR | ||||||||||
| // - Check character `s` is present in the `ASCII_CONFUSABLES` array | ||||||||||
| // | ||||||||||
| // This line equivalent to the: .filter(|(f, s)| f != s).all(|(f, s)| (ascii_confusables.contains(&f) || ascii_confusables.contains(&s))); | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is redundant, the filter representation is more confusing if anything |
||||||||||
| .all(|(f, s)| f == s || ASCII_CONFUSABLES.contains(&f) || ASCII_CONFUSABLES.contains(&s)) | ||||||||||
| // # CHECK 2 | ||||||||||
| // Compare the strings in a 'case-insensitive' comparison to avoid new allocations. | ||||||||||
| // Equivalent to: found.to_lowercase() == suggested.to_lowercase() | ||||||||||
| && found.eq_ignore_ascii_case(suggested) | ||||||||||
| // # CHECK 3 | ||||||||||
| // Ensure the found string is not equal to the suggested string. | ||||||||||
| // | ||||||||||
| // This line addressing potential issue where a code suggestion | ||||||||||
| // is generated even though it is the same as the original code. | ||||||||||
| && found != suggested | ||||||||||
| } | ||||||||||
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.
could you turn this into a let-else?