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

feat: add option to disable stripAnsi #15

Closed

Conversation

ikatyang
Copy link

Use for prettier, need this option since there may be ansi strings in the source code.

@sindresorhus
Copy link
Owner

I don't understand the use-case. How are you using this? This module is meant to measure the visual width.

@ikatyang
Copy link
Author

We always break line if the line fit the printWidth, need to measure the visual width to support CJK.

@lydell
Copy link

lydell commented Oct 11, 2017

Does it matter, though?

  • Is there really any source code that contains valid ANSI escapes (that is, not using any JavaScript escapes such as \u001b)?
  • If there is, the lines containing them will be slightly off (or you might not notice it at all). Not the end of the world.

From a correctness standpoint then, yes, we shouldn't strip ANSI in Prettier's case, but from a practical standpoint I don't think it's an issue?

@sindresorhus
Copy link
Owner

I think https://github.com/sindresorhus/string-length is what you actually want to use. This module is for text printed in the terminal.

@ikatyang
Copy link
Author

CJK characters should be measured as 2-char width (visual width):

  • stringLength("中"); //=> 1
  • stringWidth("中"); //=> 2

So I think string-width should be what we want.


We do need the feature for text printed but not in the terminal so that we (maybe) need the ability to disable the stripAnsi, but as @lydell mentioned above, it seems not a big issue since nobody writes ANSI escapes without JavaScript escapes such as \u001b.

So I'm going to close it now unless someone open an issue about this in prettier.

@sindresorhus thanks for your quick response!

@ikatyang ikatyang closed this Oct 12, 2017
@ikatyang ikatyang deleted the feat/strip-ansi-option branch October 12, 2017 03:01
@suchipi
Copy link

suchipi commented Oct 12, 2017

CJK characters should be measured as 2-char width (visual width) - @ikatyang

Why? Doesn't that mean that a markdown paragraph written using almost entirely CJK characters would break at 40 characters instead of 80? That seems surprising to me.

Do you want to treat it this way because those characters often print as slightly-wider-than-1-character due to them not being in the monospace font that the rest of the text is rendered in?

@ikatyang
Copy link
Author

CJK characters are 2-char width in monospace font.

image

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.

4 participants