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

[Console] Add support for NO_COLOR env var #34252

Merged
merged 1 commit into from
Nov 8, 2019

Conversation

Seldaek
Copy link
Member

@Seldaek Seldaek commented Nov 6, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? yes
Deprecations? no
License MIT
Doc PR

Adds support for https://no-color.org/ - ideally this would be considered a bugfix and added to older releases IMO, but submitting as new feature for now.

cc @johnstevenson

Copy link
Contributor

@johnstevenson johnstevenson left a comment

Choose a reason for hiding this comment

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

User-level configuration files and per-instance command-line arguments should override $NO_COLOR

I'm no expert here, but if I use --ansi on the command-line I would be expecting to see color output.

@nicolas-grekas nicolas-grekas added this to the 4.4 milestone Nov 6, 2019
@nicolas-grekas nicolas-grekas changed the title Add support for NO_COLOR env var [Console] Add support for NO_COLOR env var Nov 6, 2019
@nicolas-grekas
Copy link
Member

Oh, can you please add the same to
src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php and
src/Symfony/Component/VarDumper/Dumper/CliDumper.php?

@Seldaek
Copy link
Member Author

Seldaek commented Nov 6, 2019

Good point @johnstevenson - reading more in depth now I also see that this really only applies to color, and not things like backspace characters and other ANSI codes, I am not sure if the intent of the isDecorated is really just about color or about more than that?

I see in a few places like ProgressBar, non-decorated output won't use overwrite. In Composer we also assume non-decorated means no support for ANSI characters at all in some places.

So I am not so sure if it's the best way here, ideally we'd need a new API for color support independently from ANSI support. I don't know if it's best to lose all ANSI when you set NO_COLOR, or if you rather get colors anyway..

@nicolas-grekas
Copy link
Member

I think we shouldn't care about --ansi - it already has higher precedence.
Also disabling only colors but no other ansi codes would be way too challenging.
I'm good with the patch as is - it just need to be added the some more files I listed above.

@Seldaek
Copy link
Member Author

Seldaek commented Nov 8, 2019

Ok updated 👍

@johnstevenson
Copy link
Contributor

I think we shouldn't care about --ansi - it already has higher precedence.

Oops, sorry, I hadn't realized that.

@nicolas-grekas
Copy link
Member

Thank you @Seldaek.

nicolas-grekas added a commit that referenced this pull request Nov 8, 2019
This PR was merged into the 4.4 branch.

Discussion
----------

[Console] Add support for NO_COLOR env var

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| License       | MIT
| Doc PR        |

Adds support for https://no-color.org/ - ideally this would be considered a bugfix and added to older releases IMO, but submitting as new feature for now.

cc @johnstevenson

Commits
-------

c1b0a8e Add support for NO_COLOR env var
@nicolas-grekas nicolas-grekas merged commit c1b0a8e into symfony:4.4 Nov 8, 2019
@johnstevenson
Copy link
Contributor

It would be great if this could be listed in the "Libraries supporting NO_COLOR" section at https://no-color.org/ (via https://github.com/jcs/no_color)

@nicolas-grekas
Copy link
Member

@johnstevenson totally! could you please send a PR?

@Seldaek Seldaek deleted the patch-9 branch November 8, 2019 16:16
@johnstevenson
Copy link
Contributor

Hmm!

How does this look?

Software Description Version/Date Supported
Symfony PHP framework and reusable components 4.4

Which actual release will it be in?

@nicolas-grekas
Copy link
Member

Looks good!
In 4.4 yes: https://symfony.com/releases/4.4
Date: end of this month.

@stof
Copy link
Member

stof commented Nov 8, 2019

As they want a link to the release notes, maybe we should wait until the 4.4 release to be able to link to the blog post.

@johnstevenson
Copy link
Contributor

I was just about to PR this:

Software Description Version/Date Supported
Symfony PHP framework and reusable components 4.4.0

I can put the release date in the PR description, so they can decide when to merge it. Does this sound okay?

@johnstevenson
Copy link
Contributor

Sorted.

@fabpot fabpot mentioned this pull request Nov 12, 2019
@fabpot fabpot mentioned this pull request Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants