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

[color] Use graphemes to measure strings. #11202

Merged
merged 2 commits into from Nov 10, 2022
Merged

Conversation

pgavlin
Copy link
Member

@pgavlin pgavlin commented Oct 31, 2022

The number of Unicode code points in a string is not the same as the
number of user-visible characters (graphemes). When measuring colorized
strings, we want the latter rather than the former. Notably, these
changes fix some issues where the interactive display cut off before the
right edge of the terminal.

@pulumi-bot
Copy link
Contributor

pulumi-bot commented Oct 31, 2022

Changelog

[uncommitted] (2022-11-09)

Bug Fixes

  • [cli/display] Fix text cutting off prior to the edge of the terminal
    #11202

@pgavlin
Copy link
Member Author

pgavlin commented Oct 31, 2022

Stacks on top of #11201

@pgavlin pgavlin force-pushed the pgavlin/measure-graphemes branch 7 times, most recently from 652e057 to 221795e Compare November 2, 2022 15:14
@pgavlin pgavlin force-pushed the pgavlin/measure-graphemes branch 2 times, most recently from dfacf7e to 955e04c Compare November 9, 2022 01:14
The number of Unicode code points in a string is not the same as the
number of user-visible characters (graphemes). When measuring colorized
strings, we want the latter rather than the former. Notably, these
changes fix some issues where the interactive display cut off before the
right edge of the terminal.
@pgavlin pgavlin requested review from Frassle and a team November 9, 2022 16:26
<{%reset%}> <{%reset%}> │ ├─ awsx:x:ec2:NatGateway vpc-0 <{%reset%}><{%reset%}>
<{%reset%}> <{%reset%}> │ │ └─ aws:ec2:Eip vpc-0 <{%reset%}><{%reset%}>
<{%reset%}> <{%reset%}> │ ├─ awsx:x:ec2:Subnet vpc-public-0 <{%reset%}><{%reset%}>
<{%reset%}> <{%reset%}> │ │ └─ aws:ec2:RouteTable vpc-public-0 <{%reset%}><{%reset%}>
Copy link
Member

Choose a reason for hiding this comment

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

Nit: It looks like we can optimize out a colorized column of length 0 and elide it.

@pgavlin
Copy link
Member Author

pgavlin commented Nov 9, 2022

Notably, these changes fix some issues where the interactive display cut off before the right edge of the terminal.

To elaborate on these issues:

In the interactive display, resources are rendered in a tree view with visible edges from children to parents. Each of these edges is formed using Unicode Box Drawing characters. These characters are outside the ASCII range, and each of their UTF-8 code units consists of three bytes. The code in the colorizer that clamps the output of colorization to a particular width was using byte counts instead of code points or graphemes. With these changes, these measurements are now done using graphemes (i.e. user-visible characters).

<{%reset%}> <{%reset%}> ├─ aws:ec2:SecurityGroupRule cluster-eksNodeClusterIngressRule <{%reset%}><{%reset%}>
<{%reset%}> <{%reset%}> ├─ kubernetes:core/v1:ConfigMap cluster-nodeAccess <{%reset%}><{%reset%}>
<{%reset%}> <{%reset%}> ├─ aws:ec2:LaunchConfiguration cluster-nodeLaunchConfiguration <{%reset%}><{%reset%}>
<{%bold%}><{%fg 3%}>~ <{%reset%}> └─ aws:cloudformation:Stack cluster-nodes <{%bold%}><{%fg 3%}>updating<{%reset%}><{%bold%}><{%fg 3%}><{%reset%}>
Copy link
Member

Choose a reason for hiding this comment

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

Kudos: Having snapshot based tests for display logic make reviewing these PRs much easier. 👍

Copy link
Member

@AaronFriel AaronFriel left a comment

Choose a reason for hiding this comment

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

Oops. Meant to approve. Nit is non-blocking, per name.

@pgavlin
Copy link
Member Author

pgavlin commented Nov 10, 2022

bors merge

bors bot added a commit that referenced this pull request Nov 10, 2022
11202: [color] Use graphemes to measure strings. r=pgavlin a=pgavlin

The number of Unicode code points in a string is not the same as the
number of user-visible characters (graphemes). When measuring colorized
strings, we want the latter rather than the former. Notably, these
changes fix some issues where the interactive display cut off before the
right edge of the terminal.

Co-authored-by: Pat Gavlin <pat@pulumi.com>
@bors
Copy link
Contributor

bors bot commented Nov 10, 2022

Timed out.

@pgavlin
Copy link
Member Author

pgavlin commented Nov 10, 2022

bors retry

@bors
Copy link
Contributor

bors bot commented Nov 10, 2022

Build succeeded:

@bors bors bot merged commit 572912f into master Nov 10, 2022
@pulumi-bot pulumi-bot deleted the pgavlin/measure-graphemes branch November 10, 2022 19:44
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

3 participants