Attempt at fixing find_common_string #540
Conversation
|
Thanks for expanding the test. Haven't looked into the implementation details to make a Post mortem what went wrong there. |
|
I'm eager to do more work on this if you find it necessary. Honestly I'm interested myself about why exactly the original code didn't work properly but as I was reading it I couldn't really figure it out. Maybe I just need some timeout. Thanks for the review. |
|
I've checked this one again and it seems that the issue was with the check of every element against the first element. On top of that I've added a bunch of other test cases including empty haystack, haystack of one, haystack that differs at first character, etc. Hopefully I haven't gone overboard. I'll now leave this until I get further feedback. |
| // Test against haystack of size 1 | ||
| let input: Vec<_> = source | ||
| .into_iter() | ||
| .take(1) | ||
| .map(|s: &str| Suggestion { | ||
| value: s.into(), | ||
| description: None, | ||
| extra: None, | ||
| span: Span::new(0, s.len()), | ||
| append_whitespace: false, | ||
| }) | ||
| .collect(); | ||
| let res = find_common_string(&input); | ||
|
|
||
| match res { | ||
| (Some(first), Some(index)) => assert!( | ||
| first.value == input.first().unwrap().value && index == first.value.len()), | ||
| _ => assert!(false, "There should be a result with both options as some with one element to check against"), | ||
| } |
There was a problem hiding this comment.
This whole pattern is repeated, can we factor it out so the tests are more readable and we are more confident what the differences between the tests are (and avoid introducing a bug in the test fixture)
| let mut new_index: Option<usize> = match first { | ||
| Some(first) => Some(first.value.len()), | ||
| None => Some(0), | ||
| }; |
There was a problem hiding this comment.
That's a .map(...).unwrap_or_default()
| if values.len() <= 1 { | ||
| return (first, new_index); | ||
| } |
There was a problem hiding this comment.
Maybe worth adding a comment what the different cases are. We have this early return and the chain from hell below.
This PR attempts to fix issue #539
Description
I suspect it may have to do with the way map works, so I've done the check differently. But I'm new with Rust so there's probably a better option.
I've also updated the test case so that it shows the issue that it previously didn't.
To-Do
After seeing reviews for this PR I may also check the test function for non_ansi characters.