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

[refactor] use termcolor crate instead of ansi #156

Open
HerringtonDarkholme opened this issue Jan 3, 2023 · 15 comments
Open

[refactor] use termcolor crate instead of ansi #156

HerringtonDarkholme opened this issue Jan 3, 2023 · 15 comments
Labels
refactor Better engineering refactor

Comments

@HerringtonDarkholme
Copy link
Member

Using ansi will make some windows client unhappy because cygwin like terminals does not support ansi but using console api.

@pd4d10
Copy link
Contributor

pd4d10 commented May 13, 2023

Is this refactor started? If not I'd like to take it

@HerringtonDarkholme
Copy link
Member Author

Nope, it is not started. Happy to see you are interested but would you like to provide a high-level plan for the refactor?
It will be quite huge.

@pd4d10
Copy link
Contributor

pd4d10 commented May 13, 2023

Sure, I'll look into it.

@pd4d10
Copy link
Contributor

pd4d10 commented May 16, 2023

After doing some research, I found that the usage of termcolor is a little different from other libraries (with just ANSI support), because it aims to support the Windows console API, which also means it seems difficult to log texts with different styles in one same statement. For example:

// the styles of header and message are different
writeln!(w, "{header} {message}");

should be firstly refactored into

write!(w, header);
writeln!(w, " {message}");

So I think the refactor could be split into two steps:

  1. Following the example above, refactor all the existing print stuff.
  2. Finish the ansi_term -> termcolor replacement.

The trade-off is that we would lose some intuitiveness when doing print stuff in the future.

What do you think?

@HerringtonDarkholme
Copy link
Member Author

Nice writeup! I wonder if we can test the huge refactor accordingly? If the end user is not impacted by the change, I would opt for termcolor since it also supports windows console API.

@pd4d10
Copy link
Contributor

pd4d10 commented May 16, 2023

if we can test the huge refactor accordingly

Yeah, we can firstly improve the test coverage, which is currently 60% according to the badge at README, then do the refactor.

@HerringtonDarkholme
Copy link
Member Author

Yeah! Sure!

@pd4d10
Copy link
Contributor

pd4d10 commented May 17, 2023

It seems that we already have tests, but there is no check for the color. For example

mod test {
use super::*;
#[test]
fn test_display_error() {
let error = anyhow::anyhow!("test error").context(ErrorContext::ReadConfiguration);
let error_fmt = ErrorFormat {
context: &ErrorContext::ReadConfiguration,
inner: &error,
};
let display = format!("{error_fmt}");
assert_eq!(display.lines().count(), 6);
assert!(display.contains("Cannot read configuration."));
assert!(
display.contains("Caused by"),
"Should display the error chain"
);
assert!(display.contains("test error"));
}
#[test]
fn test_bare_anyhow() {
let error = anyhow::anyhow!(ErrorContext::ReadConfiguration);
let error_fmt = ErrorFormat {
context: &ErrorContext::ReadConfiguration,
inner: &error,
};
let display = format!("{error_fmt}");
assert_eq!(display.lines().count(), 3);
assert!(display.contains("Cannot read configuration."));
assert!(
!display.contains("Caused by"),
"Should not contain error chain"
);
}
}

I guess we can use snapshot to avoid hard code such as "\u1b[31" in the test cases. A lib named insta seems to meet our needs.

Shall we use it or just do the hard code in test cases, what do you think?

@RReverser
Copy link

Would this also help with fixing color support on Windows? Currently --color says it default to auto which is described as

      - auto:   Try to use colors, but don't force the issue. If the output is piped to another program, or the console isn't available on Windows, or if TERM=dumb, or if `NO_COLOR` is defined, for example, then don't use colors

but I don't know what "console isn't available on Windows" means. I'm on Windows 11 and I always have to manually provide sg --color ansi to get colors in Windows Terminal as well as VSCode terminal, which gets a bit annoying to have to remember.

@HerringtonDarkholme
Copy link
Member Author

Would this also help with fixing color support on Windows?

I do know if using termcolor should somehow fix Windows usage. Windows Terminal and VSCode terminal accepts ansi color so --color ansi.

I believe termcolor supports Windows console API which supports color natively without using ANSI escape code. Unfortunately, I don't have Windows machine to test the behavior of termcolor :(

I would try to refactor usage in future so external contributors can test termcolor.

@RReverser
Copy link

RReverser commented Oct 24, 2023

Windows Terminal and VSCode terminal accepts ansi color

More specifically, it's accepted by Windows itself (in ConPTY component which other terminals build on) since Windows 10, not by individual apps. Unless you want to support old Windows versions (which hopefully shouldn't be necessary for developers), it makes just defaulting to ANSI colors a much easier cross-platform option.

@RReverser
Copy link

There is some Windows API you'll still need to check if colors are supported (e.g. to make sure it's not a redirect or something), but I believe one of these crates should already handle that for you.

@HerringtonDarkholme
Copy link
Member Author

Sounds good. I will investigate it. Changing default color is tracked by #680

@HerringtonDarkholme
Copy link
Member Author

HerringtonDarkholme commented Nov 15, 2023

ansi color is the default color option in Windows since 0.13.1!

@RReverser
Copy link

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Better engineering refactor
Projects
Status: Not Started
Development

No branches or pull requests

3 participants