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

Replace nu-ansi-term with anstyle crates #2567

Closed
wants to merge 5 commits into from

Conversation

nickelc
Copy link
Contributor

@nickelc nickelc commented May 12, 2023

The nu-ansi-term crate can now be replaced by the anstyle crates which are used by clap for its ANSI text styling.

@nickelc
Copy link
Contributor Author

nickelc commented May 12, 2023

So, I looked into the failing regression tests and the difference is that anstyle writes the effects for bold, italic etc. separately.

playground

@sharkdp
Copy link
Owner

sharkdp commented Sep 1, 2023

Thank you and sorry for the review delay. Can you please go into more details why this is necessary/beneficial?

@Enselic
Copy link
Collaborator

Enselic commented Sep 1, 2023

I think the benefit is that we get rid of a dependency (nu-ansi-term) without adding any new ones, since the "new" ones are already needed by clap?

Enselic
Enselic previously approved these changes Sep 1, 2023
Copy link
Collaborator

@Enselic Enselic left a comment

Choose a reason for hiding this comment

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

This looks good to me. The ANSI color diffs all seem to be only differences in how the colors are encoded, not how they are displayed (at least in my terminal I did not see any actual color change).

Maybe there is a mistake among all those lines. That can be fixed with a follow up PR if needed. When I glance it over the code changes slowly it all looks reasonable to me.

Can you rebase this on latest master please?

@sharkdp
Copy link
Owner

sharkdp commented Sep 1, 2023

So, I looked into the failing regression tests and the difference is that anstyle writes the effects for bold, italic etc. separately.

This could have adverse effects though, right? We need to write more output. And terminal emulators might take a longer time to parse the input and/or to set up the proper style.

I would appreciate if we could run a benchmark to see that this doesn't have any negative effects. Since we want to include possible effects of the terminal emulator, we can use hyperfines --show-output option.

@Enselic Enselic dismissed their stale review September 1, 2023 19:33

Let's run performance checks first

@Enselic
Copy link
Collaborator

Enselic commented Sep 1, 2023

Good point about performance. I didn't think about that. Let's see some numbers before we merge.

@Enselic Enselic added the needs-benchmarking We need to check for regressions in performance before we can merge this. label Sep 2, 2023
The crate is already pulled in by `clap`.
The `anstyle` crate is already used by `clap` to print ANSI text styles.
The `anstyle` crate emits the escape codes for the bold, italic, etc. separately.

Now, it is `\x1B[3m\x1B[38;2;102;217;239m` for `RGB(102, 217, 239)` and italic
instead of `\x1B[3;38;2;102;217;239m`.
@Enselic
Copy link
Collaborator

Enselic commented Dec 1, 2023

Closing for now to keep the PR inbox clean. Of course feel free to re-open if you resume work on this!

@Enselic Enselic closed this Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-benchmarking We need to check for regressions in performance before we can merge this.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants