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

Remove Crayons per issue 3741, use click echo functions instead #5178

Merged
merged 5 commits into from
Jul 19, 2022
Merged

Remove Crayons per issue 3741, use click echo functions instead #5178

merged 5 commits into from
Jul 19, 2022

Conversation

otherjake
Copy link
Contributor

@otherjake otherjake commented Jul 18, 2022

Thank you for contributing to Pipenv!

The issue

Remove crayons per issue 3741
Fixes #3741

The fix

Use click.echo, click.secho, click.style instead

The checklist

  • support colorblind envV with click.style if possible

@matteius matteius requested review from oz123 and matteius July 18, 2022 16:27
@matteius
Copy link
Member

I think in the long run we need to deprecate the colorblind environment variable and replace it with a more appropriately named variable. However we do need to consider that we likely had already broken that variable with the current usages of click styling. @oz123 can you think of a way to support disabling the click styling at a global level in pipenv based on enabling a specific flag?

@oz123
Copy link
Contributor

oz123 commented Jul 18, 2022

I think in the long run we need to deprecate the colorblind environment variable and replace it with a more appropriately named variable. However we do need to consider that we likely had already broken that variable with the current usages of click styling. @oz123 can you think of a way to support disabling the click styling at a global level in pipenv based on enabling a specific flag?

The appropriate name would be NO_COLOR, see here: https://no-color.org/

rich, which is already used by pip supports it. In the long run, I would like us to use the same infrastructure as pip, hence I would vote in favor using coloring, and progress bars from rich. It's more work, but it is more sustainable for us.

As for globally disabling click colors as long we use it, we can steal some code from the click-extras project and monkey patch click to disable the color function. This already supports the familiar no color.

Side note; colorblinded users can simply disable colors globally in their tty or use tee:

$ pipenv | tee

This was originally printed with "yellow" foreground.
@oz123 oz123 merged commit 20ab154 into pypa:main Jul 19, 2022
if environments.PIPENV_COLORBLIND:
crayons.disable()
#if environments.PIPENV_COLORBLIND:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the spot where it previously switched the NO_COLOR behavior

Copy link
Contributor

@oz123 oz123 Jul 20, 2022

Choose a reason for hiding this comment

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

Here is a monkey patch which we can use to support NO_COLOR

image

Also, for backwards compatibility, I would support both PIPENV_COLORBLIND and NO_COLOR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am implementing a conditional monkeypatch in this spot on style and secho, they are used separately for composing messages that aren't all one color, also tests for each for the old and new variables. Should an issue be created for deprecating PIPENV_COLORBLIND?

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.

Remove crayons from pipenv in favor of click.styles
3 participants