Skip to content

#39: Add color to varify deps status results#79

Merged
dpc merged 1 commit intocrev-dev:masterfrom
ffranr:prettify_verify
Dec 24, 2018
Merged

#39: Add color to varify deps status results#79
dpc merged 1 commit intocrev-dev:masterfrom
ffranr:prettify_verify

Conversation

@ffranr
Copy link
Member

@ffranr ffranr commented Dec 19, 2018

No description provided.

@dpc
Copy link
Collaborator

dpc commented Dec 19, 2018

Concept OK, but colored won't work on windows. Also, it will print ascii garbage when users pipe the outputs to file etc.

We should use term crate, which is much more pain to use, because it can't abstract thing away as Strings, but that's because on windows colors are drawn by calling some kernel APIs, and not using ansi escape strings etc.

You can use https://github.com/slog-rs/term/blob/master/src/lib.rs#L1380 as a reference on how to use term.

You should probably implement something like this:

impl VerificationStatus {
 fn write_colored_to_stdout(&self) -> io::Result<()> { ... }
 fn write_colored_to_stderr(&self) -> io::Result<()> { ... }
}

Inside each of them, detect if the given output is a terminal (isatty) and if it is, use term calls to set and reset the color.

@ffranr
Copy link
Member Author

ffranr commented Dec 19, 2018

Thanks for the feedback! I'll take a look at term.

@ffranr
Copy link
Member Author

ffranr commented Dec 24, 2018

@dpc Is this the sort of thing you were thinking of?
I've used term.supports_color(). Do you think that I need to query isatty from libc also?

@dpc dpc merged commit b5f28b7 into crev-dev:master Dec 24, 2018
@dpc
Copy link
Collaborator

dpc commented Dec 24, 2018

I have landed (that's my philosophy), but there are problems:

See cargo crev verify deps | less. The output is corrupted because we need to use something like https://docs.rs/atty/0.2.11/atty/ to detect if the output is actually an interactive terminal that supports colors. term.supports_color() must be checking something else. I'm guessing if$TERM variable supports color?

Some other improvements:

There will be more stuff to output in color.

We should probably have an API like this:

trait Colorizable { // or some better name
   fn color(&self) -> SomeTypeForColor;
}

and then some generic function/struct that checks term.supports_color() and atty and given an argument of type T : Colorizable + Display outputs it on stderr or stdout. The checks should probably be cached (I'm not sure how expensive they are).

@rffrancon Thank you for this PR, and let me know if you plan to work on the above, or should I look into it eventually, when I find a time.

@dpc
Copy link
Collaborator

dpc commented Dec 24, 2018

I'm going to take a quick stab at it.

@ffranr
Copy link
Member Author

ffranr commented Dec 26, 2018

@dpc Thank you for the feedback once more! I see that you've pushed this much further than what I had in mind, looks great. :)

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.

2 participants