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

Don't use Ansi Colors if not supported. #523

Merged
merged 4 commits into from
Apr 13, 2022
Merged

Conversation

DW-Ernest
Copy link
Contributor

We are using this tool in an environment that doesn't support ANSI Color code. So we have made a small change to enable them only when the environment supports them.

image

We referred to this website to construct the support_ansi_colors function
https://bixense.com/clicolors/

@jpy-git
Copy link

jpy-git commented Mar 31, 2022

I have this issue as well so would love to see this merged ☝️

@jpdakran
Copy link
Member

jpdakran commented Apr 1, 2022

This looks good to me. Can you add some test coverage for this file? That will help get our test coverage up to par here. Also has this been tested on a windows machine? I am still trying to setup windows in our CI pipeline so we do not have that coverage.

@DW-Ernest
Copy link
Contributor Author

I added a test coverage for utils/color. It's the first time I'm using pytest, so please tell me if I can improve the test.
I made and tried it on an Ubuntu machine. But else, I am using detect-secrets on Windows. I never tried to run the code test on Windows though.

tests/util/color_test.py Outdated Show resolved Hide resolved
tests/util/color_test.py Outdated Show resolved Hide resolved
tests/util/color_test.py Outdated Show resolved Hide resolved
@jpdakran
Copy link
Member

jpdakran commented Apr 5, 2022

The pytest implementation looks good. I just suggested possibly a few function name changes to describe what the test is doing. Thanks for your contribution!

@lorenzodb1
Copy link
Member

Thanks for your contribution! I'd wait until we push #528 so we can further test this against Windows before formally approving it.

@lorenzodb1
Copy link
Member

Closing and re-opening this PR to run the additional Windows checks from #528.

@lorenzodb1 lorenzodb1 closed this Apr 13, 2022
@lorenzodb1 lorenzodb1 reopened this Apr 13, 2022
@lorenzodb1 lorenzodb1 merged commit c1d2902 into Yelp:master Apr 13, 2022
@lorenzodb1
Copy link
Member

Thank you for your contribution @DW-Ernest!

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

4 participants