-
Notifications
You must be signed in to change notification settings - Fork 203
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
Changes from all commits
00f42da
fec24d0
4a36add
ba5b584
9c16e65
1afd964
d81f9d0
7f24e7f
f926706
3d9f5e2
04554cf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,11 @@ | ||
| //! Collection of common functions that can be used to create menus | ||
| use std::borrow::Cow; | ||
| use unicase::UniCase; | ||
|
|
||
| use itertools::{ | ||
| FoldWhile::{Continue, Done}, | ||
| Itertools, | ||
| }; | ||
| use nu_ansi_term::{ansi::RESET, Style}; | ||
| use unicode_segmentation::UnicodeSegmentation; | ||
|
|
||
|
|
@@ -168,39 +173,28 @@ pub fn parse_selection_char(buffer: &str, marker: char) -> ParseResult<'_> { | |
| } | ||
|
|
||
| /// Finds index for the common string in a list of suggestions | ||
| pub fn find_common_string(values: &[Suggestion]) -> (Option<&Suggestion>, Option<usize>) { | ||
| let first = values.iter().next(); | ||
|
|
||
| let index = first.and_then(|first| { | ||
| values.iter().skip(1).fold(None, |index, suggestion| { | ||
| if suggestion.value.starts_with(&first.value) { | ||
| Some(first.value.len()) | ||
| pub fn find_common_string(values: &[Suggestion]) -> Option<(&Suggestion, usize)> { | ||
ysthakur marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| let first_suggestion = values.first()?; | ||
| let max_len = first_suggestion.value.len(); | ||
|
|
||
| let index = values | ||
| .iter() | ||
| .skip(1) | ||
| .fold_while(max_len, |cumulated_min, current_suggestion| { | ||
| let new_common_prefix_len = first_suggestion | ||
| .value | ||
| .char_indices() | ||
| .zip(current_suggestion.value.chars()) | ||
| .find_map(|((idx, lhs), rhs)| (rhs != lhs).then_some(idx)) | ||
| .unwrap_or(max_len); | ||
| if new_common_prefix_len == 0 { | ||
| Done(0) | ||
| } else { | ||
| first | ||
| .value | ||
| .char_indices() | ||
| .zip(suggestion.value.char_indices()) | ||
| .find(|((_, mut lhs), (_, mut rhs))| { | ||
| lhs.make_ascii_lowercase(); | ||
| rhs.make_ascii_lowercase(); | ||
|
|
||
| lhs != rhs | ||
|
Comment on lines
-184
to
-187
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. I think this should be case sensitive, correct me if I'm wrong. |
||
| }) | ||
| .map(|((new_index, _), _)| match index { | ||
| Some(index) => { | ||
| if index <= new_index { | ||
| index | ||
| } else { | ||
| new_index | ||
| } | ||
| } | ||
| None => new_index, | ||
| }) | ||
| Continue(cumulated_min.min(new_common_prefix_len)) | ||
| } | ||
| }) | ||
| }); | ||
| }); | ||
|
|
||
| (first, index) | ||
| Some((first_suggestion, index.into_inner())) | ||
| } | ||
|
|
||
| /// Finds different string between two strings | ||
|
|
@@ -345,15 +339,17 @@ pub fn replace_in_buffer(value: Option<Suggestion>, editor: &mut Editor) { | |
|
|
||
| /// Helper for `Menu::can_partially_complete` | ||
| pub fn can_partially_complete(values: &[Suggestion], editor: &mut Editor) -> bool { | ||
| if let (Some(Suggestion { value, span, .. }), Some(index)) = find_common_string(values) { | ||
| let index = index.min(value.len()); | ||
| if let Some((Suggestion { value, span, .. }, index)) = find_common_string(values) { | ||
| let matching = &value[0..index]; | ||
| let end = floor_char_boundary(editor.get_buffer(), span.end); | ||
| let start = floor_char_boundary(editor.get_buffer(), span.start).min(end); | ||
|
|
||
| // make sure that the partial completion does not overwrite user entered input | ||
| let extends_input = matching.starts_with(&editor.get_buffer()[start..end]) | ||
| && matching != &editor.get_buffer()[start..end]; | ||
| let entered_input = &editor.get_buffer()[start..end]; | ||
| let extends_input = UniCase::new(matching) | ||
| .to_folded_case() | ||
| .contains(&UniCase::new(entered_input).to_folded_case()) | ||
| && matching != entered_input; | ||
|
Comment on lines
+348
to
+352
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. And this should be case insensitive... |
||
|
|
||
| if !matching.is_empty() && extends_input { | ||
| let mut line_buffer = editor.line_buffer().clone(); | ||
|
|
@@ -718,46 +714,23 @@ mod tests { | |
| assert_eq!(res, (1, "e")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn find_common_string_with_ansi() { | ||
| use crate::Span; | ||
|
|
||
| let input: Vec<_> = ["nushell", "null"] | ||
| .into_iter() | ||
| .map(|s| Suggestion { | ||
| value: s.into(), | ||
| description: None, | ||
| style: None, | ||
| extra: None, | ||
| span: Span::new(0, s.len()), | ||
| append_whitespace: false, | ||
| ..Default::default() | ||
| }) | ||
| .collect(); | ||
| let res = find_common_string(&input); | ||
|
|
||
| assert!(matches!(res, (Some(elem), Some(2)) if elem == &input[0])); | ||
| } | ||
|
|
||
| #[test] | ||
| fn find_common_string_with_non_ansi() { | ||
| use crate::Span; | ||
|
|
||
| let input: Vec<_> = ["nushell", "null"] | ||
| #[rstest] | ||
| #[case::ascii(vec!["nushell", "null"], 2)] | ||
| #[case::non_ascii(vec!["nushell", "null"], 6)] | ||
| // https://github.com/nushell/nushell/pull/16765#issuecomment-3384411809 | ||
| #[case::unsorted(vec!["a", "b", "ab"], 0)] | ||
| #[case::should_be_case_sensitive(vec!["a", "A"], 0)] | ||
ysthakur marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| fn test_find_common_string(#[case] input: Vec<&str>, #[case] expected: usize) { | ||
| let input: Vec<_> = input | ||
| .into_iter() | ||
| .map(|s| Suggestion { | ||
| value: s.into(), | ||
| description: None, | ||
| style: None, | ||
| extra: None, | ||
| span: Span::new(0, s.len()), | ||
| append_whitespace: false, | ||
| ..Default::default() | ||
| }) | ||
| .collect(); | ||
| let res = find_common_string(&input); | ||
| let (_, len) = find_common_string(&input).unwrap(); | ||
|
|
||
| assert!(matches!(res, (Some(elem), Some(6)) if elem == &input[0])); | ||
| assert!(len == expected); | ||
| } | ||
|
|
||
| #[rstest] | ||
|
|
@@ -803,11 +776,7 @@ mod tests { | |
| replace_in_buffer( | ||
| Some(Suggestion { | ||
| value, | ||
| description: None, | ||
| style: None, | ||
| extra: None, | ||
| span: Span::new(start, end), | ||
| append_whitespace: false, | ||
| ..Default::default() | ||
| }), | ||
| &mut editor, | ||
|
|
||
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
.gitattributesand add this as a generated file in there, not sure how we haven't done that until now