-
Notifications
You must be signed in to change notification settings - Fork 192
fix: wrong answer of find_common_string
#969
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
| lhs.make_ascii_lowercase(); | ||
| rhs.make_ascii_lowercase(); | ||
|
|
||
| lhs != rhs |
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 this should be case sensitive, correct me if I'm wrong.
| let entered_input = &editor.get_buffer()[start..end]; | ||
| let extends_input = matching | ||
| .to_ascii_lowercase() | ||
| .contains(&entered_input.to_ascii_lowercase()) | ||
| && matching != entered_input; |
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.
And this should be case insensitive...
25f01b1 to
3d9f5e2
Compare
|
@ysthakur Would you please take a look at this when you get time? |
ysthakur
left a comment
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, LGTM, just need a couple more test cases
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.
Unrelated to this PR, but we should make a .gitattributes and add this as a generated file in there, not sure how we haven't done that until now
ysthakur
left a comment
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.
LGTM. The new unicase dependency only adds 23.5 KiB, which I think is acceptable
Fixes a bug described here.
Update:
Fixes nushell/nushell#15535