Add test for insert only suggestion #107
Conversation
* Also remove the ensure that prevents handling of insert only suggestions
src/replace.rs
Outdated
| @@ -75,6 +70,8 @@ impl Data { | |||
| self.original.len() | |||
| ); | |||
|
|
|||
| let insert_only = from > up_to_and_including; | |||
There was a problem hiding this comment.
i feel like this should check that its== from+1 and the ensure above should check for that, i think ranges where from is 2 or more are still invalid, not sure if its worth testing for that or its one of those "I assume this will actually never happen" things.
src/replace.rs
Outdated
| @@ -135,7 +143,8 @@ impl Data { | |||
| if from > part_to_split.start { | |||
| new_parts.push(Span { | |||
| start: part_to_split.start, | |||
| end: from, | |||
| // end: if insert_only { from - 1 } else { from }, | |||
* Also adds a test for invalid ranges, specifically looking for ranges that are neither replace nor insert, as in the exclusive_end is less than the start.
There was a problem hiding this comment.
Very nice!
Looking at CI, it seems that proptest found some issues with replace::tests::new_to_vec_roundtrip, though (a quickcheck-like tool for property testing, you can run it with cargo test --all -- --ignored).
The first problems are easily fixed by changing the x - 1s to x.saturating_sub(1)s, which I've commented on in the diff. Another error is Test failed: index 1 out of range for slice of length 0; minimal failing input: ref s = "" which gets triggered in the to_vec method.
src/replace.rs
Outdated
| @@ -36,7 +37,7 @@ impl Data { | |||
| Span { | |||
| data: State::Initial, | |||
| start: 0, | |||
| end: data.len(), | |||
| end: data.len() - 1, | |||
There was a problem hiding this comment.
Let's replace this with data.len().saturating_sub(1) so it can never underflow (0.saturating_sub(1) == 0)
src/replace.rs
Outdated
| @@ -135,7 +152,7 @@ impl Data { | |||
| if from > part_to_split.start { | |||
| new_parts.push(Span { | |||
| start: part_to_split.start, | |||
| end: from, | |||
| end: from - 1, | |||
There was a problem hiding this comment.
Let's replace this with from.saturating_sub(1) so it can never underflow
src/replace.rs
Outdated
| } else { | ||
| false | ||
| } | ||
| }; |
There was a problem hiding this comment.
Nice to see the closure work! Looking at how it is used here, it might be more idiomatic to write this as a method on State, though
suggestions