Skip to content

[Console][PhpUnitBridge][VarDumper] Add support for FORCE_COLOR environment variable #57777

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 13, 2024
Merged

[Console][PhpUnitBridge][VarDumper] Add support for FORCE_COLOR environment variable #57777

merged 1 commit into from
Aug 13, 2024

Conversation

serious-angel
Copy link
Contributor

@serious-angel serious-angel commented Jul 19, 2024 •
edited
Loading

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
Issues
License MIT

@carsonbot carsonbot added this to the 7.2 milestone Jul 19, 2024
serious-angel
serious-angel commented Jul 19, 2024
View reviewed changes
Copy link
Contributor Author

@serious-angel serious-angel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure about CHANGELOG file and how breaking it is.
Please suggest any details if possible.

Related: chalk/supports-color#105

@nicolas-grekas
Copy link
Member

Related also: https://force-color.org/

@nicolas-grekas
Copy link
Member

BTW, https://no-color.org/ tells that the NO_COLOR env var should not be the empty string, and we don't follow this spec currently. I think we should.

@carsonbot carsonbot changed the title Added support for FORCE_COLOR environment variable [VarDumper] Added support for FORCE_COLOR environment variable Jul 19, 2024
@OskarStark OskarStark changed the title [VarDumper] Added support for FORCE_COLOR environment variable [VarDumper] Add support for FORCE_COLOR environment variable Jul 19, 2024
@alexandre-daubois alexandre-daubois mentioned this pull request Jul 24, 2024
Merged
nicolas-grekas added a commit that referenced this pull request Jul 26, 2024
@nicolas-grekas
bug #57815 [Console][PhpUnitBridge][VarDumper] Fix NO_COLOR empty v…
d4c681c 
…alue handling (alexandre-daubois)

This PR was merged into the 5.4 branch.

Discussion
----------

[Console][PhpUnitBridge][VarDumper] Fix `NO_COLOR` empty value handling

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | #57777 (comment)
| License       | MIT

`NO_COLOR` must be non-empty in order to be considered enabled (https://no-color.org/):

> when present and not an empty string (regardless of its value)

Commits
-------

6a96ff9 [Console][PhpUnitBridge][VarDumper] Fix `NO_COLOR` empty value handling
symfony-splitter pushed a commit to symfony/var-dumper that referenced this pull request Jul 26, 2024
@nicolas-grekas
bug #57815 [Console][PhpUnitBridge][VarDumper] Fix NO_COLOR empty v…
0c17c56 
…alue handling (alexandre-daubois)

This PR was merged into the 5.4 branch.

Discussion
----------

[Console][PhpUnitBridge][VarDumper] Fix `NO_COLOR` empty value handling

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | symfony/symfony#57777 (comment)
| License       | MIT

`NO_COLOR` must be non-empty in order to be considered enabled (https://no-color.org/):

> when present and not an empty string (regardless of its value)

Commits
-------

6a96ff9116 [Console][PhpUnitBridge][VarDumper] Fix `NO_COLOR` empty value handling
@alexandre-daubois
Copy link
Member

Hi @artshade, you may have a look at this fix to be sure to update everywhere it is relevant in Symfony: #57815

@serious-angel
Copy link
Contributor Author

...you may have a look at this fix to be sure to update everywhere it is relevant in Symfony: #57815

This is awesome! Thank you, @alexandre-daubois ! Have you considered the related forced color option?

@alexandre-daubois
Copy link
Member

You seemed to be well on your way to adding this feature, so I didn't want to impose a PR if you wanted to finish it :)

@serious-angel serious-angel force-pushed the patch-1 branch 2 times, most recently from 45133e8 to 9cd9c63 Compare July 29, 2024 21:32
@serious-angel
Copy link
Contributor Author

serious-angel commented Jul 29, 2024 •
edited
Loading

You seemed to be well on your way to adding this feature, so I didn't want to impose a PR if you wanted to finish it :)

Changed the priority of NO_COLOR as in the snippet at https://force-color.org, where NO_COLOR comes first and only then - FORCE_COLOR:

if (no_color != NULL && no_color[0] != '\0')
    color = false;
// ...
if (force_color != NULL && force_color[0] != '\0')
    color = true;

Source

Regarding the PR, I am frankly not sure if the current state of the PR is correct considering all the branches and rules.

derrabus
derrabus requested changes Jul 29, 2024
View reviewed changes
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

Thank you. Please add a test.

@carsonbot carsonbot changed the title [VarDumper] Add support for FORCE_COLOR environment variable [Console][PhpUnitBridge][VarDumper] Add support for FORCE_COLOR environment variable Aug 13, 2024
@nicolas-grekas
Copy link
Member

Thank you @artshade.

@nicolas-grekas nicolas-grekas merged commit 8fb48a5 into symfony:7.2 Aug 13, 2024
4 of 11 checks passed
@serious-angel
Copy link
Contributor Author

I am sorry for not implementing the test in-time...
Sincere gratitude, @nicolas-grekas, and everyone...

Let it be colors! 🌈 ✨

@serious-angel serious-angel deleted the patch-1 branch August 13, 2024 17:02
@fabpot fabpot mentioned this pull request Oct 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derrabus derrabus derrabus requested changes

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

Assignees
No one assigned
Labels
Console Feature PhpUnitBridge Status: Needs Review VarDumper
Projects
None yet
Milestone
7.2
Development

Successfully merging this pull request may close these issues.
6 participants
@serious-angel @nicolas-grekas @alexandre-daubois @derrabus @OskarStark @carsonbot