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

Wrap termcolor's color api #75

Merged
merged 2 commits into from
Mar 18, 2018
Merged

Conversation

KodrAus
Copy link
Collaborator

@KodrAus KodrAus commented Mar 10, 2018

Noted in #72

The pub use termcolor::Color we released env_logger 0.5.0 with was a bad call.

This PR duplicates the termcolor::Color API in env_logger so breaking changes in termcolor can be managed with some gymnastics in env_logger.

If a new variant is added to termcolor::Color:

  • Color::from_termcolor will return None until/unless we have a corresponding variant
  • Color::FromStr will return Err::Unrecognized until/unless we have a corresponding variant

If termcolor drops the concept of Color completely 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 ParseColorError isn't equivalent to termcolor::ParseColorError so you couldn't ? it and Color isn't equivalent to termcolor::Color, but pragmatically I think this is safe to make in a patch.

cc @BurntSushi @briansmith what do you think?

@KodrAus KodrAus force-pushed the feat/wrap-termcolor branch from e98b5a6 to 2863544 Compare March 10, 2018 23:57
@briansmith
Copy link

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 Style API that is only available when termcolor is used, so that all styling code (not just colors) is avoided when no styling is wanted.

However, again, this is a positive change, even if I don't think it is optimal.

@KodrAus
Copy link
Collaborator Author

KodrAus commented Mar 12, 2018

You're right, I think we can do things differently in env_logger with more freedom over the API next time we want to make breaking changes to env_logger.

This PR pulls in the complete API that's available now through pub use termcolor::Color, so we can make termcolor a private dependency in a patch.

I think the next incremental step would be to move all color and styling APIs into a submodule under fmt (even if it's just re-exported) so it'll be easier to conditionally exclude it.

@briansmith
Copy link

You're right, I think we can do things differently in env_logger with more freedom over the API next time we want to make breaking changes to env_logger.

This is a breaking change already, right? It is possible (perhaps unlikely) that somebody is already relying on the fact that env_logger::Color is the same type as termcolor::Color.

@KodrAus
Copy link
Collaborator Author

KodrAus commented Mar 12, 2018

It is technically a breaking change. I looked through env_logger's public dependents using Color and none of them seem to depend on env_logger::Color and termcolor::Color being the same (we've never advertised Color as coming from termcolor). So I'm comfortable pushing it through as a patch with details in the release notes unless folks are deeply opposed, in which case it might be a while before we're ready to bundle it with other breaking changes.

@KodrAus KodrAus merged commit f32b662 into rust-cli:master Mar 18, 2018
@KodrAus KodrAus deleted the feat/wrap-termcolor branch March 18, 2018 01:25
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