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

Output colors support? #2

Closed
sebasmagri opened this issue Jul 6, 2017 · 10 comments
Closed

Output colors support? #2

sebasmagri opened this issue Jul 6, 2017 · 10 comments
Milestone

Comments

@sebasmagri
Copy link
Collaborator

sebasmagri commented Jul 6, 2017

As per log's review. Check out pretty_env_logger.
Issue in log

@budziq
Copy link
Contributor

budziq commented Jul 11, 2017

I'd love such feature.

Unfortunately I see is no way to support color output in cross platform way without changing the architecture somewhat (Builder::format having to return a String).
https://github.com/seanmonstar/pretty-env-logger shows colored output only on ANSI compliant terminals (as there you just need to add ansi control codes to the console). I've encountered this issue when working on https://github.com/azerupi/mdBook/pull/369

@KodrAus
Copy link
Collaborator

KodrAus commented Oct 30, 2017

We should be able to support basic colour support now that the output isn't a plain string.

I'll have a play with it using term-painter and see where it gets us. Are there any other recommendations for terminal colouring libs?

@lnicola
Copy link
Contributor

lnicola commented Oct 30, 2017

I've also seen https://github.com/BurntSushi/ripgrep/tree/master/termcolor and https://github.com/SergioBenitez/yansi.

@KodrAus
Copy link
Collaborator

KodrAus commented Oct 30, 2017

Hmm, termcolor looks interesting, thanks I'll dig into it! In classic @BurntSushi style it looks like it solves all of life's problems.

@KodrAus
Copy link
Collaborator

KodrAus commented Oct 30, 2017

Hmm, so the whole termcolor API is pretty interesting. Looks like Buffers + WriteColor could neatly solve this and #31 too, but means a new public dependency.

I'll spend some time spiking it out.

@lnicola
Copy link
Contributor

lnicola commented Oct 30, 2017

It seems pretty lightweight, with no extra dependencies except on Windows.

@KodrAus
Copy link
Collaborator

KodrAus commented Oct 30, 2017

Ah sorry I think half that comment stayed in my head.

If we want to support colours using termcolor then we'd need to change our format function from accepting a Box<Write> to accepting a Box<WriteColor> or something. In that case we're publicly exposing termcolor's API and need to be careful about upgrading it. That might not actually be a problem though. Just something I want to keep in mind.

It does look like a pretty self-contained crate though, so if this works out I don't think I'd have a problem depending on it in env_logger.

@BurntSushi
Copy link
Contributor

I'm happy to see you're considering termcolor, but I would be careful of making it a public dep. I've never been particularly happy with its API. In particular, I think it is somewhat cumbersome to use. I think the idea of a WriteColor trait and the buffering strategy for multithreaded support is probably here to stay, but I'm particularly open to changing how colors are set and such.

Unfortunately, I don't have any specific plans as of yet to improve the API, but at this stage in the game I'm somewhat pretty relaxed about doing a semver bump on it.

@KodrAus
Copy link
Collaborator

KodrAus commented Oct 30, 2017

Thanks for your thoughts @BurntSushi! If the API is still a bit unstable then we probably should avoid exposing it publicly, since I hope we can keep the next major release of env_logger around as long as the next major release of log.

I'm pretty sold on the Buffer API, which we can use privately for #31. For colours I'll play with introducing a concrete structure for the format writer rather than a trait object so we've got some more control and can at least provide some private methods for the default format to use colours.

@BurntSushi
Copy link
Contributor

@KodrAus Sounds good!

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

No branches or pull requests

5 participants