Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unconditional wrapping #30

Merged
merged 2 commits into from
Nov 26, 2020

Conversation

erikgeiser
Copy link
Contributor

This PR adds unconditional wrapping where each line is ended as soon as the limit is reached. The code is tested and documented in README.md. The PR also closes #29.

fmt.Println(wrap.String("Hello World!", 7))
// Hello W
// orld!

This also works great in conjunction with word-wrapping when word-wrapping is preferred but the limit is mandatory (e.g. terminal width):

fmt.Println(wrap.String(wordwrap.String("Just an example", 5), 5))
// Just
// an
// examp
// le

The unconditional wrapper also allows for some configuration:

  • Newline: Like in word-wrap
  • KeepNewlines: Like in word-wrap
  • PreserveSpace: Whether leading spaces in new lines should be ignored
  • TabWidth: Width of tabs when expresses as spaces (because respecting the limit is impossible otherwise)

Also I noticed a common pattern when checking ANSI markers, so I added helpers to the ansi package. These can also be applied to the other algorithms in another PR.

@muesli
Copy link
Owner

muesli commented Nov 25, 2020

Thanks for this PR, @erikgeiser!

I like it and it's indeed much needed, but I wonder if it wouldn't be nicer to make the unconditional wrapping an option in the existing wordwrap package. What do you think?

@erikgeiser
Copy link
Contributor Author

I think there might be cases where no word-wrapping is desired but the output should still be within a limit. In this case it would be awkward to be able to configure wordwrap in a way that does not do word-wrapping at all.

However, having an option in wordwrap to enable unconditional wrapping in addition to word-wrapping is a really good idea because I guess it would be commonly used this way. So maybe the best option is to keep wrap and add an option to wordwrap that uses the wrap package.

Also, after using it a bit I think the PerserveSpace option should work a bit differently than I currently implemented it. I wanted an option determines whether spaces at the beginning of a newline after a forceful linebreak should be stripped. This is how it works but it would be better if it never stripped leading spaces after a newline in input, because then the user actually wanted the spaces to be there.

@muesli
Copy link
Owner

muesli commented Nov 25, 2020

Ok, I'm convinced, that sounds rather reasonable. Do you want to fix the PreserveSpace behavior in this PR?

Adding an option to the wordwrap package could be a separate PR. I'm not quite sure how we would pass the wrap packages' options to it, in that case. Since both packages provide the same function signatures, though, they should be fairly interchangeable as it is.

@erikgeiser
Copy link
Contributor Author

Yes, i'll fix it in this PR. But the name PreserveSpace is a bit confusing when it behaves this way. Maybe I'll rename it PreserveSpacesAfterForcefulLinebreak. What do you think?

@muesli
Copy link
Owner

muesli commented Nov 25, 2020

Maybe it should be the other way around and called TrimWhitespace and trim spaces on both ends? Seems a bit more concise. I would also suggest enabling this for the default Writer.

@erikgeiser
Copy link
Contributor Author

I'm not sure if we are thinking about the same concept.

In my opionion the default behavior should be this:

Input:
foo bar

Output (limit 3):
foo
bar

or in the case where the user broke the line:

Input:
foo bar
  baz

Output (limit 3):
foo
bar
 ba
z

When disabling this option it should look like this:

Input:
foo bar

Output (limit 3):
foo
 ba
r

or in the case where the user broke the line:

Input:
foo bar
  baz

Output (limit 3):
foo
 ba
r
 ba
z

@muesli
Copy link
Owner

muesli commented Nov 25, 2020

I see how that could be useful, too. Especially for source code. But I wonder, for human-readable text I would prefer this output (with the option enabled):

Input:
foo bar
  baz

Output (limit 3):
foo
bar
baz

@erikgeiser
Copy link
Contributor Author

erikgeiser commented Nov 26, 2020

I can see where you are coming from but why should wrapping affect spaces that were put there intentionally by the user? In my opinion, the option is about removing spaces that have become redundant by the wrapping and not for general cleaning. If users want the behavior that you just showed, they can still strip leading spaces out of their lines before passing it to wrap.

I primarily implemented wrap to be used in charmbracelet/bubbles#25. If you look at the videos, you see that the selection options are indented and I wouldn't want this indentation to go away just because I also want wrapping if the line is too long.

@muesli muesli added the enhancement New feature or request label Nov 26, 2020
@muesli muesli merged commit eee88dd into muesli:master Nov 26, 2020
@muesli
Copy link
Owner

muesli commented Nov 26, 2020

Thank you for this contribution, Erik!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add unconditional (non-word) wrapping
2 participants