Make indent preserve existing newlines in the input string #279

jRimbault · 2021-01-06T12:22:03Z

I ported a custom iterator to get the inclusive split feature specifically with a char (though it could be even be just a custom iterator with \n as a const value).

This fixes #207 on stable rust but I'd feel safer once we're able to use the much more generic split_inclusive, hopefully.

~~It uses unsafe code. Which is forbidden in this crate.~~ It's up to you @mgeisler if you want to make an exception or not.

Before, indent("foo", "") would give "foo\n". It now preserves any trailing newline character present in the input string. This makes indent behave consistently with dedent. New tests ere added to ensure this on a number of corner cases. closes mgeisler#207

I guess this causes bound checking, but I think I prefer that.

mgeisler · 2021-01-12T12:12:52Z

~~It uses unsafe code. Which is forbidden in this crate.~~ It's up to you @mgeisler if you want to make an exception or not.

Hey @jRimbault, thanks for removing the unsafe code again! Until someone shows up with a performance problem (with benchmarks...), there's no need for unsafe code here 😄

.github/workflows/build.yml

src/indentation.rs

tests/indent.rs

jRimbault · 2021-01-16T14:00:16Z

I'll get on to it soon :)

mgeisler · 2021-01-16T15:20:57Z

Hi @jRimbault, no stress! That's the beauty of open source 😄

mgeisler · 2021-01-16T15:21:05Z

... and thanks!

…-iter

jRimbault · 2021-01-20T09:48:58Z

I tried making some fuzz work but I don't understand quite what it's testing for ? Panics ? The examples here don't seem to actually test things ?

In any case I think your solution to simply add an enumerating index is really quite good since it doesn't need bumping the MSRV (split_inclusive will be part of 1.51). I also think, rather than fuzzing, taking tests from tried and proved libraries like, this crate inspiration, python's textwrap should be a better faster option (CPU time, and reliability wise).

mgeisler · 2021-01-21T13:25:00Z

I tried making some fuzz work but I don't understand quite what it's testing for ? Panics ? The examples here don't seem to actually test things ?

Yes, you raise a good point and you guessed correctly: they simply test that the code doesn't panic 😄 I should really document this...

In any case I think your solution to simply add an enumerating index is really quite good since it doesn't need bumping the MSRV (split_inclusive will be part of 1.51). I also think, rather than fuzzing, taking tests from tried and proved libraries like, this crate inspiration, python's textwrap should be a better faster option (CPU time, and reliability wise).

Fair enough, I like it a lot that you ported those tests. They give us a nice foundation and we can then always think about turning them into property-based tests later. You're definitely right that it's much cheaper to have some solid unit tests for this.

tests/indent.rs

jRimbault · 2021-01-21T13:30:31Z

Happy to help 😄

Textwrap behavior changed after mgeisler/textwrap#279

Textwrap behavior changed after mgeisler/textwrap#279 Closes: #7705

Textwrap behavior changed after mgeisler/textwrap#279 Closes: diem#7705

jRimbault added 3 commits September 30, 2020 18:54

add custom iterator

946d9d5

remove unsafe blocks

75ced97

I guess this causes bound checking, but I think I prefer that.