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

feat: make is_terminal replaceble with std implementation #160

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

JarvisCraft
Copy link

Description

This adds an opt-out legacy-is-terminal feature whose absence enables the use of std's IsTerminal instead of is-termminal's.

This way users of Rust 1.70+ will be able to remove is-terminal's dependency tree from their dependencies while not breaking.

The only potential breakage is for users who both:

  • use Rust <1.70;
  • disable default features;
  • have this dependency as a transitive.

Which seems to be very rare (if observable at all).

This is to follow @pinkforest's suggestion that MSRV bump to 1.70 cannot be done for now.

Alternatives

Considering that (any) change requires a version bump anyway, I would prefer the feature to be opt-out (i.e. use std by default and only fall back to is-terminal if explicitly required by a non-default feature) which would be more logical and more supportive towards those who use newer Rust versions.

@pinkforest
Copy link
Collaborator

pinkforest commented Nov 22, 2023

features are only additive and this hands compilation control to transient dependencies that is not what is favorable.

In worst case if done as non-additive these are prone to cause miscompilation and maintenance is a headache.

Majority of big downstream are transient libraries and not top-level binaries leaving problematic feature-chain via tree.

The correct way would be to use cfg if the intent is to provide switchable implementations.

Can see relevant discussion here:

There we had a problem where these features were ~never used or in worst case it led to compilation errors and dependency chain generally never adopted chaining them across to top-level binary.

Also this would still raise MSRV (no default features) and making something like this on-by-default is not going to work with downstream - features weren't really designed to be used to "negate" compilation either.

However the impact of having the dependency is minor and I do not see a reason to add additional complexity temporarily.

The other PR can stay as csv bump but this can switch to isTerminal given when we are open to move up MSRV.

@JarvisCraft
Copy link
Author

Apologies for the late reply, I've initially read the message but did not respond at the time


features are only additive and this hands compilation control to transient dependencies that is not what is favorable.

In worst case if done as non-additive these are prone to cause miscompilation and maintenance is a headache.

This is the exact reason why this specific PR implements this via an enabled-by-default "legacy" (is-terminal crate based): in order to enable the "breaking" std-based implementation the default legacy-is-terminal should be disable (via default-features = false) in all dependency tree members. If any of them does not do this, the original (crate-based) implementation is used.

Also this would still raise MSRV (no default features) and making something like this on-by-default is not going to work with downstream - features weren't really designed to be used to "negate" compilation either.

That's true.

The correct way would be to use cfg if the intent is to provide switchable implementations.

That's a nice Idea! I will try to re-implement this PR using it, thanks for the reference.

The other PR can stay as csv bump but this can switch to isTerminal given when we are open to move up MSRV.

Are there any plans or schedule for the MSRV bump? Not trying to be insitent here, but just planning on when to come with a PR using this ability to get rid of legacy :)

@JarvisCraft JarvisCraft marked this pull request as draft November 30, 2023 19:56
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