Make Pretty.break tail recursive #476
Conversation
Do you have an example of a failing test? |
|
Not in a form I could share easily. In short it's a A bit longer story is that while the commands themselves are short all the messages are kept in and thus a lot longer when written out as a scalacheck is writing out these states upon test failure, which then causes This might be a rare occurrence, and maybe our tests are badly designed, but regardless, I think it shouldn't end in a StackOverflow from the framework. |
The motivation for this is typelevel#476. I wanted to ensure we had stronger tests for Pretty.break before refactoring it, and also thought that this implementation was a little bit nicer. This PR includes a test for stack-safety, a test to ensure we never create lines longer than the maximum allowed, and that we can reassemble the original string from the output. The old code passed all of these (apart from the stack safety test which it failed), which helps give confidence and that the new implementation is equivalent. It also includes a Scaladoc method to explain what the method does. Thanks to @GytisZ for reporting the issue and providing an initial PR.
|
I ended up writing some tests to nail down the break behavior, and ended up with a slightly different stack-safe refactor: |
Hi, we have a rather large test, that would trigger stack overflow from
Prettybreak. This should fix it.Tried to keep to similar style. The only functional change is the added requirement for
lead.length.No tests submitted, since it seems
util.Prettyis not tested a lot. I'm leaning towards adding them, but don't want change too much, so waiting for yay/nay whether to do it.