WIP incorrect_line_wrapping_endash #37

asdf · 2019-09-11T05:40:40Z

asdf commented

2019-09-11 05:40:40 +00:00

This PR is being submitted for a possible fix to line wrapping with unicode characters.

The version currently submitted has 3 versions of wrapLines() for performance review. The final version would have this removed.

The performance benchmark tests should run from bombadillo/cui directory as go test -bench=.

This PR is being submitted for a possible fix to line wrapping with unicode characters. The version currently submitted has 3 versions of `wrapLines()` for performance review. The final version would have this removed. The performance benchmark tests should run from `bombadillo/cui` directory as `go test -bench=.`

sloum was assigned by asdf

2019-09-11 10:27:18 +00:00

sloum reviewed 2019-09-11 17:38:32 +00:00

cui/cui.go Outdated

sloum commented

2019-09-11 17:38:32 +00:00

Looking at the benchmarks this seems to get the best combination of speed and function (len on its own being faster). I like the count rune in string method for its clarity, if we go with this maybe add a code comment for why we are casting to rune. I could see myself forgetting, or someone looking at it not knowing in the first place.

If you agree with the assessment, please remove the other two and update the branch and I'll complete the pull request into master :)
Otherwise, let me know and we can talk out the other options.

Looking at the benchmarks this seems to get the best combination of speed and function (len on its own being faster). I like the count rune in string method for its clarity, if we go with this maybe add a code comment for why we are casting to rune. I could see myself forgetting, or someone looking at it not knowing in the first place. If you agree with the assessment, please remove the other two and update the branch and I'll complete the pull request into master :) Otherwise, let me know and we can talk out the other options.

asdf commented

2019-09-14 22:55:52 +00:00

I've finalised this PR and suggest it be merged for v1 or canned in anticipation of v2.

When wrapping lines, this measures characters in a more correct way, so wrapping is more accurate.

There are other issues with line wrapping that are not addressed by this PR, but are addressed in v2. Also, no new tests have been added as they do not reliably test this behaviour.

I've finalised this PR and suggest it be merged for v1 or canned in anticipation of v2. When wrapping lines, this measures characters in a more correct way, so wrapping is more accurate. There are other issues with line wrapping that are not addressed by this PR, but are addressed in v2. Also, no new tests have been added as they do not reliably test this behaviour.

sloum commented

2019-09-15 00:57:23 +00:00

Looks good to me! I think continuing to make fixes like this, particularly for things where we've talked through options and found a workable solution, is still a good thing to do for v1. It is possible that someone may have a preference for v1 over v2 once it comes out and I'd like them to have the best experience possible. :)

This code looks good. Merging in now 👍

Looks good to me! I think continuing to make fixes like this, particularly for things where we've talked through options and found a workable solution, is still a good thing to do for v1. It is possible that someone may have a preference for v1 over v2 once it comes out and I'd like them to have the best experience possible. :) This code looks good. Merging in now :thumbsup:

sloum closed this pull request

2019-09-15 00:57:31 +00:00

Sign in to join this conversation.

No reviewers