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 option to not remove whitespace - fixes #9 #17

Merged
merged 5 commits into from
Jul 23, 2017
Merged

Add option to not remove whitespace - fixes #9 #17

merged 5 commits into from
Jul 23, 2017

Conversation

SamVerschueren
Copy link
Contributor

This PR tries to solve issue #9 where wrap-ansi removes leading and trailing whitespace by default.

Although the tests succeed, and log-update works again as expected, I don't think it's really the expected output that we get in the tests.

When I highlight the output, this is what I see

screen shot 2017-06-13 at 22 17 58

I don't think I should have those extra whitespace character (1 in green, 1 in red). I believe they should be added to the next line instead. So I will have a look at how I can solve that.

But please, feedback is more than appreciated! For instance, should we have extend the trim option to also accept leading, trailing so that users can choose to only remove trailing or only remove leading whitespaces? log-update for instance should have enough by only trimming the trailing whitespace.

// @SBoudrias @sindresorhus @Qix- @bcoe

@SamVerschueren
Copy link
Contributor Author

SamVerschueren commented Jun 13, 2017

Found a way to have it like this

screen shot 2017-06-13 at 22 49 46

But the solution is really dirty IMO so I want to explore other options first. But to be sure, is this what it should look like?

Note, it is a highlighted screenshot to show the start and end of the ANSI escape codes. This is the real output.

screen shot 2017-06-13 at 22 53 16

@SamVerschueren
Copy link
Contributor Author

I came a bit further in solving this with PR #19.

@keevitaja
Copy link

Thank you. Nice pull request! I was about to write my own word-wrapper until saw this.

@sindresorhus
Copy link
Member

I came a bit further in solving this with PR #19.

Now that #19 is merged, is this PR still up to date?

@sindresorhus sindresorhus changed the title add option to not remove whitespace - fixes #9 Add option to not remove whitespace - fixes #9 Jul 21, 2017
@SamVerschueren
Copy link
Contributor Author

I think it is. But let me verify when I'm on my computer. It's been a while since the PR :).

@SamVerschueren
Copy link
Contributor Author

SamVerschueren commented Jul 21, 2017

Still have some additional work for this as the current solution does not make the cut at the correct position. It should look like this right?

screen shot 2017-06-13 at 22 53 16

Then I can adjust the test and do the additional work.

@sindresorhus
Copy link
Member

It should look like this right?

Yes

@SamVerschueren
Copy link
Contributor Author

Fixed it with the last commit. I believe this should do it.

@sindresorhus
Copy link
Member

@SamVerschueren Sorry, you have a merge conflict after #20.

@SamVerschueren
Copy link
Contributor Author

No problem, should be fixed now.

@sindresorhus
Copy link
Member

Awesome! Hopefully we can finally get log-update fixed now :)

@SamVerschueren
Copy link
Contributor Author

Let me check that :).

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

Successfully merging this pull request may close these issues.

None yet

4 participants