disable Force color over no color by Aditya-PS-05 · Pull Request #8183 · astral-sh/uv

Aditya-PS-05 · 2024-10-14T18:28:22Z

zanieb · 2024-10-14T18:31:39Z

crates/uv/src/settings.rs

+            } else if matches!(
+                args.color,
+                ColorChoice::Always | ColorChoice::Never | ColorChoice::Auto
+            ) {
+                // If `--color` is passed explicitly, use the value of `args.color` directly.
+                args.color


Won't this always match since the default is "auto" at the clap-level?

Yupp, But I thought if --color is passed explicitly, we can use the value of args.color directly.

Also, when I tested the expected result was coming for all three separate cases. Means for FORCE_COLOR and --color and with sub arguments.

Won't this always match since the default is "auto" at the clap-level?

Done purposely, as when the user does not anything (neither the environment variables nor the --color), then I wanted the auto property. As If --color is given, it doesn't matter the value, we can always use them.

samypr100 · 2024-10-14T18:42:53Z

Unless I'm missing something, I believe this changes standard behavior since env var should still take priority over cli when honoring the order of no color > force color | force cli color > auto

For example, if you have NO_COLOR set and pass --color always as well, NO_COLOR should take precedence.

zanieb · 2024-10-14T20:08:28Z

if you have NO_COLOR set and pass --color always as well, NO_COLOR should take precedence.

Should it? Honestly not sure.

samypr100 · 2024-10-14T20:24:17Z

if you have NO_COLOR set and pass --color always as well, NO_COLOR should take precedence.

Should it? Honestly not sure.

Fair, I was considering CI scenarios when I said this. Wouldn't this be a breaking if changed?

charliermarsh · 2024-10-14T20:25:38Z

The docs (https://no-color.org/) at least state that "per-instance command-line arguments should override the NO_COLOR environment variable".

samypr100 · 2024-10-14T20:27:07Z

The docs (https://no-color.org/) at least state that "per-instance command-line arguments should override the NO_COLOR environment variable".

Does the same apply for FORCE_COLOR? Or CLICOLOR_FORCE?

samypr100 · 2024-10-14T20:31:55Z

The docs (https://no-color.org/) at least state that "per-instance command-line arguments should override the NO_COLOR environment variable".

Does the same apply for FORCE_COLOR? Or CLICOLOR_FORCE?

Sounds like it does at least for CLICOLOR_FORCE, https://bixense.com/clicolors/
FORCE_COLOR doesn't say.

I've likely been using it wrong 😆

To me, its a bit vague since the relationship between NO_COLOR and FORCE_COLOR and CLICOLOR_FORCE in this scenario is not well defined in any of the standards from what I can see.

Sounds like uv should follow this then?

Explicit CLI --color never / --color always takes precedence over the env vars.
Env vars then kick-in, NO_COLOR > FORCE_COLOR | CLICOLOR_FORCE
Auto Detection

What about an explicit --color auto? It's not clear which one would apply in the presence of the env vars being set.

charliermarsh · 2024-10-14T23:36:42Z

Yeah, agree with this @samypr100:

Explicit CLI --color never / --color always takes precedence over the env vars.
Env vars then kick-in, NO_COLOR > FORCE_COLOR | CLICOLOR_FORCE
Auto Detection

I think --color=auto should also override the env vars (https://bixense.com/clicolors/ shows an example of that, arguably).

charliermarsh · 2024-10-15T12:26:50Z

@Aditya-PS-05 - Do you plan to pursue the follow-up changes mentioned here?

Aditya-PS-05 · 2024-10-15T12:29:24Z

Yeah, agree with this @samypr100:
* Explicit CLI --color never / --color always takes precedence over the env vars.

* Env vars then kick-in, NO_COLOR > FORCE_COLOR | CLICOLOR_FORCE

* Auto Detection
I think --color=auto should also override the env vars (https://bixense.com/clicolors/ shows an example of that, arguably).

@charliermarsh
I am making this change following your instructions.

Aditya-PS-05 · 2024-10-15T13:28:04Z

All the required changes are done in the pr #8215

zanieb reviewed Oct 14, 2024

View reviewed changes

Aditya-PS-05 closed this Oct 15, 2024

zanieb mentioned this pull request Oct 15, 2024

Improve interactions between color environment variables and CLI options #8215

Merged

Comments

Conversation

Aditya-PS-05 commented Oct 14, 2024

Uh oh!

zanieb Oct 14, 2024

Choose a reason for hiding this comment

Uh oh!

Aditya-PS-05 Oct 14, 2024

Choose a reason for hiding this comment

Uh oh!

Aditya-PS-05 Oct 14, 2024

Choose a reason for hiding this comment

Uh oh!

Aditya-PS-05 Oct 14, 2024

Choose a reason for hiding this comment

Uh oh!

samypr100 commented Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zanieb commented Oct 14, 2024

Uh oh!

samypr100 commented Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

charliermarsh commented Oct 14, 2024

Uh oh!

samypr100 commented Oct 14, 2024

Uh oh!

samypr100 commented Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

charliermarsh commented Oct 14, 2024

Uh oh!

charliermarsh commented Oct 15, 2024

Uh oh!

Aditya-PS-05 commented Oct 15, 2024

Uh oh!

Aditya-PS-05 commented Oct 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

samypr100 commented Oct 14, 2024 •

edited

Loading

samypr100 commented Oct 14, 2024 •

edited

Loading

samypr100 commented Oct 14, 2024 •

edited

Loading