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: support NO_COLOR and FORCE_COLOR env vars #3979

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

samypr100
Copy link
Collaborator

@samypr100 samypr100 commented Jun 3, 2024

Summary

Closes #3955

Adds explicit support to NO_COLOR and FORCE_COLOR via GlobalSettings.

The order, per specs is now NO_COLOR > FORCE_COLOR > color.

This PR is a backup plan pending rust-cli/anstyle#192.

Test Plan

Tested all cases locally for now; I didn't see existing tests for GlobalSettings parsing.

Copy link

codspeed-hq bot commented Jun 3, 2024

CodSpeed Performance Report

Merging #3979 will not alter performance

Comparing samypr100:adjust-color-settings (affba38) with main (40e0ddd)

Summary

✅ 13 untouched benchmarks

@charliermarsh
Copy link
Member

Should we add CLICOLOR_FORCE as well so that it's respected over the color CLI argument?

@charliermarsh
Copy link
Member

I'm fine to merge this regardless of anstream changes.

crates/uv/src/cli.rs Outdated Show resolved Hide resolved
@samypr100 samypr100 force-pushed the adjust-color-settings branch from b8c4a56 to 2c2fbb7 Compare June 4, 2024 02:05
@samypr100 samypr100 force-pushed the adjust-color-settings branch from 2c2fbb7 to 1c451f6 Compare June 4, 2024 02:05
crates/uv/src/settings.rs Outdated Show resolved Hide resolved
Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

@samypr100 - This looks good to me. Ok to merge?

@samypr100 samypr100 marked this pull request as ready for review June 4, 2024 20:35
@charliermarsh charliermarsh merged commit 1b3200b into astral-sh:main Jun 4, 2024
46 checks passed
@charliermarsh charliermarsh added configuration Settings and such cli Related to the command line interface labels Jun 4, 2024
@samypr100 samypr100 deleted the adjust-color-settings branch June 4, 2024 21:35
@bluss
Copy link
Contributor

bluss commented Jun 5, 2024

The way I read the examples on those two pages, https://force-color.org/ http://no-color.org/ they both suggest that command line flags that turn on color should have precedence over NO_COLOR. i.e "do getopt(3) and/or config-file parsing to possibly turn color back on" and so on. Not sure if I'm confusing myself, or if there are contradicting specs.

@samypr100
Copy link
Collaborator Author

samypr100 commented Jun 5, 2024

@bluss They're examples, not the standard. The standard itself (the sentence at the top of the pages) doesn't go into detail on how they are meant to interact with each other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to the command line interface configuration Settings and such
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support FORCE_COLOR and NO_COLOR
5 participants