Wrap termcolor's color api#75
Conversation
e98b5a6 to
2863544
Compare
|
I think this is a positive change. My main motivation with this kind of change is eliminating the termcolor dependency or at least making it optional. This appears to be progress in achieving that. My more general motivation for this kind of change is eliminating complexity. IMO, rather than have a more complete color API in env_logger to avoid the termcolor dependency, it would be better to have a simpler API such that, if somebody chooses to enable the termcolor dependency, then they get the color API, but if they don't choose that dependency, then they just don't get the color API and all color parsing and whatever is just a no-op. In fact, I wish the API were different so that all the styling were only available through a However, again, this is a positive change, even if I don't think it is optimal. |
|
You're right, I think we can do things differently in This PR pulls in the complete API that's available now through I think the next incremental step would be to move all color and styling APIs into a submodule under |
This is a breaking change already, right? It is possible (perhaps unlikely) that somebody is already relying on the fact that |
|
It is technically a breaking change. I looked through |
Noted in #72
The
pub use termcolor::Colorwe releasedenv_logger 0.5.0with was a bad call.This PR duplicates the
termcolor::ColorAPI inenv_loggerso breaking changes intermcolorcan be managed with some gymnastics inenv_logger.If a new variant is added to
termcolor::Color:Color::from_termcolorwill returnNoneuntil/unless we have a corresponding variantColor::FromStrwill returnErr::Unrecognizeduntil/unless we have a corresponding variantIf
termcolordrops the concept ofColorcompletely then we have a little more work to do, but it should still all be manageable.This will technically be a breaking change, since our
ParseColorErrorisn't equivalent totermcolor::ParseColorErrorso you couldn't?it andColorisn't equivalent totermcolor::Color, but pragmatically I think this is safe to make in a patch.cc @BurntSushi @briansmith what do you think?