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

Replace atty with is_terminal #10

Merged
merged 1 commit into from
Dec 15, 2022
Merged

Replace atty with is_terminal #10

merged 1 commit into from
Dec 15, 2022

Conversation

pacak
Copy link
Contributor

@pacak pacak commented Nov 25, 2022

Fixes #9

@pacak
Copy link
Contributor Author

pacak commented Dec 15, 2022

Any thoughts?

@zkat
Copy link
Owner

zkat commented Dec 15, 2022

Oh sorry, I don't know how this fell off my list.

Copy link
Owner

@zkat zkat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM. It's unfortunate that it has to be a semver-major change, but that's my own fault for having re-exported atty types like that. Thank you!

@zkat zkat merged commit edf565e into zkat:main Dec 15, 2022
@Porges
Copy link
Contributor

Porges commented Dec 20, 2022

I would argue that since the API is breaking anyway the exposed function should accept any IsTerminal, which makes the on function more general and flexible.

@pacak
Copy link
Contributor Author

pacak commented Dec 20, 2022

I would argue that since the API is breaking anyway the exposed function should accept any IsTerminal, which makes the on function more general and flexible.

Too late for that I'm afraid. Using IsTerminal required more changes to cached version of supports color request. Doable, but more complex and it would couple supports-color to is_terminal.

@Porges
Copy link
Contributor

Porges commented Dec 20, 2022

I would argue that since the API is breaking anyway the exposed function should accept any IsTerminal, which makes the on function more general and flexible.

Too late for that I'm afraid. Using IsTerminal required more changes to cached version of supports color request. Doable, but more complex and it would couple supports-color to is_terminal.

Maybe a feature-gated alternative function would be useful?

@zkat
Copy link
Owner

zkat commented Dec 20, 2022

I definitely feel a bit weird that all the other supports-* crates would end up using IsTerminal, but supports-color wouldn't. I'm trying to figure out what would be best. I don't think it's inherently bad to couple to is-terminal if it's going to be a more permanent fixture in the cargo ecosystem, tbh.

@Porges
Copy link
Contributor

Porges commented Dec 21, 2022

I definitely feel a bit weird that all the other supports-* crates would end up using IsTerminal, but supports-color wouldn't. I'm trying to figure out what would be best. I don't think it's inherently bad to couple to is-terminal if it's going to be a more permanent fixture in the cargo ecosystem, tbh.

It will at some point be replaced by std::io::IsTerminal, so maybe the best thing to do is to expose the Stream-enum API for now and then switch to the trait when it is no longer unstable.

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.

atty is potentially unsound, not very alive and should be replaced with is-terminal
3 participants