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(console): add unicodeWidth for TTY text layout #3297

Merged
merged 10 commits into from
Apr 18, 2023

Conversation

lionel-rowe
Copy link
Contributor

@lionel-rowe lionel-rowe commented Mar 31, 2023

Closes #3296.

A TypeScript port of the unicode-width Rust crate, which looks up the nominal width of Unicode characters. This allows calculating the expected physical column width of a string in TTY-like environments when combined with stripColor from fmt/colors.ts.

@lionel-rowe lionel-rowe requested a review from kt3k as a code owner March 31, 2023 10:49
@lionel-rowe lionel-rowe force-pushed the string_width branch 2 times, most recently from fe9d454 to e3795a5 Compare April 1, 2023 05:08
@iuioiua
Copy link
Collaborator

iuioiua commented Apr 2, 2023

I'm -1 on this PR as I don't believe it's worth the added complexity of a Rust implementation. We recently deprecated the Rust implementation of encoding/varint for this same reason.

I'm also unsure whether this functionality is sought after enough to be in the Standard Library. I don't feel strongly about this, though.

That's not to say such functionality would be valuable in a 3rd party module! 🙂

@lionel-rowe
Copy link
Contributor Author

I don't believe it's worth the added complexity of a Rust implementation

To clarify, this is a TypeScript port of a Rust crate, not a WASM implementation. The Rust code is just a thin wrapper around the crate, used for checking equivalency of results during testing, and isn't imported by the module code itself.

@lionel-rowe lionel-rowe marked this pull request as draft April 3, 2023 03:26
@lionel-rowe lionel-rowe marked this pull request as ready for review April 3, 2023 09:52
string_width/_strip_ansi.ts Outdated Show resolved Hide resolved
string_width/README.md Outdated Show resolved Hide resolved
string_width/string_width.ts Outdated Show resolved Hide resolved
@kt3k
Copy link
Member

kt3k commented Apr 4, 2023

std/string_width top level module sounds too specific to me. How about std/console (or something similar)?

@kt3k
Copy link
Member

kt3k commented Apr 4, 2023

string_width/_scripts/generateData.ts

We prefer _tools over _scripts. Also we usually use snake case for file names (generateData.ts -> generate_data.ts

@lionel-rowe lionel-rowe changed the title feat(string_width): add stringWidth for TTY text layout feat(console): add unicodeWidth for TTY text layout Apr 6, 2023
@kt3k
Copy link
Member

kt3k commented Apr 13, 2023

The implementation looks good to me.

@kt3k
Copy link
Member

kt3k commented Apr 14, 2023

It's bikeshedding topic, but I'm wondering between 2 api name candidates: stringWidth and unicodeWidth. Does anyone have preference between these? cc @iuioiua @lino-levan @timreichen @jsejcksn

@lionel-rowe
Copy link
Contributor Author

lionel-rowe commented Apr 14, 2023

It's bikeshedding topic, but I'm wondering between 2 api name candidates: stringWidth and unicodeWidth

FWIW, while I don't think the name unicodeWidth is ideal as it could perpetuate the myth that Unicode strings are some special type of strings distinct from "ASCII strings", it at least gives a hint at the type of functionality it provides (grabbing width data for the constituent characters as specified by Unicode), while not implying it does something it doesn't (e.g. stripping ANSI codes to give a more accurate physical width in TTYs). The first version of this PR used the name stringWidth as it combined both functions, until you pointed out that the ANSI-stripping functionality is already provided in fmt/colors.js.

@lino-levan
Copy link
Contributor

It's bikeshedding topic, but I'm wondering between 2 api name candidates: stringWidth and unicodeWidth

I've done a bit of pondering (my original response before I cut it down to this was ~half a page long), but I think that stringWidth makes the most sense to me. At the end of the day, this method needs to be discoverable and I think that even if stringWidth isn't the best description of what it doesn't do, it is a very clear description of what it does. unicodeWidth is harder to search for and the functionality (imo) isn't immediately obvious. That being said, I don't feel super strongly either way. Just my 2 cents.

@jsejcksn
Copy link
Contributor

It's bikeshedding topic, but I'm wondering between 2 api name candidates: stringWidth and unicodeWidth. Does anyone have preference between these?

I'm in favor of keeping unicodeWidth.

  • Socratic: What's the difference between string width and string length?

    The term width is ambiguous/overloaded. The usage here is very technical, and, for that reason alone, I think it makes sense to use unicode because it helps disambiguate.

  • This is a port and I think porting the name is sensible as well — there's nothing special about the context of Deno which invalidates it (compared to other ports where idioms don't translate), and keeping the name will give confidence to Deno users who are already familiar with the upstream library.

@timreichen
Copy link
Contributor

stringWidth seems more familiar to me, but I don't have a strong opinion.

kt3k and others added 3 commits April 17, 2023 18:56
Co-authored-by: Jesse Jackson <jsejcksn@users.noreply.github.com>
console/unicode_width_test.ts Outdated Show resolved Hide resolved
Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for your contribution @lionel-rowe

Also thanks all for discussions and reviews

@kt3k kt3k merged commit 71e05aa into denoland:main Apr 18, 2023
7 checks passed
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.

Add string width calculation for TTY-based text layout
6 participants