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

honor CLICOLOR and CLICOLOR_FORCE #302

Closed
wants to merge 6 commits into from
Closed

Conversation

lelongg
Copy link

@lelongg lelongg commented Feb 17, 2023

add support of CLICOLOR to disable color output and CLICOLOR_FORCE to force color output

See https://bixense.com/clicolors/

@Miuler
Copy link

Miuler commented Mar 28, 2023

resolve #307

@bconn98
Copy link
Collaborator

bconn98 commented Nov 30, 2023

@lelongg Are you able to resolve the conflicts on this branch?

@lelongg
Copy link
Author

lelongg commented Nov 30, 2023

it is resolved

bconn98
bconn98 previously approved these changes Nov 30, 2023
@bconn98
Copy link
Collaborator

bconn98 commented Dec 2, 2023

@estk Recommend this MR to control color vs no color. Using ENV vars is more standardized than a configuration flag recommended by #305.

@estk
Copy link
Owner

estk commented Dec 3, 2023

@lelongg excellent work.
@bconn98 I think both could be helpful but lets see if this works well enough to satisfy the author of #305. As always thanks again for your help.

@lelongg

  1. Regarding the failing test, it'd be great to have a solution for windows, but if not no worries feel free to either shut up clippy or #cfg(not(windows)) the var.
  2. Can we also implement NO_COLOR? If not no worries.
  3. Can you write tests for this (impl is excellent and simple, but it'll keep us from breaking things in the future.)

@lelongg
Copy link
Author

lelongg commented Dec 19, 2023

I submitted points 1 and 2. About the tests I'm not sure how to handle this, I didn't find examples of tests checking the logs output in the code, can you help ?

@estk
Copy link
Owner

estk commented Dec 19, 2023

No problem, I would create a new example in the examples dir that utilizes (or does not utilize) the feature.
Then create an integration test (in /test) that uses std::process::Command to run the example.
Finally you will verify its output is as you expect.

@codecov-commenter
Copy link

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (58b92c8) 61.12% compared to head (b536024) 60.81%.

Files Patch % Lines
src/encode/writer/console.rs 39.28% 17 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #302      +/-   ##
==========================================
- Coverage   61.12%   60.81%   -0.32%     
==========================================
  Files          23       23              
  Lines        1407     1429      +22     
==========================================
+ Hits          860      869       +9     
- Misses        547      560      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bconn98
Copy link
Collaborator

bconn98 commented Dec 20, 2023

@lelongg Take a look at my branch #335 which makes the changes for windows. I'm not sure the right way to test this in the CI though (or in windows). I tested the linux version running locally but I don't have a windows env to test in. I didn't take @estk's suggestion on the integration example with std::process yet, instead I took the lazy way out in the CI.

@bconn98
Copy link
Collaborator

bconn98 commented Dec 20, 2023

Interesting, I swapped over to a new test using exe. It works fine locally but weird results in the CI, but CLICOLOR_FORCE works!

@bconn98
Copy link
Collaborator

bconn98 commented Jan 26, 2024

@lelongg Looks like #335 was merged in. If you're happy with the changes, please close the PR.

@lelongg lelongg closed this Feb 7, 2024
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.

None yet

5 participants