Skip to content

Conversation

@hugovk
Copy link
Member

@hugovkhugovk commented Dec 12, 2024

When -E is passed (and therefore sys.flags.ignore_environment is true), we should only ignore only env vars matching PYTHON*; in this case, only PYTHON_COLORS.

We should not ignore the other env vars: FORCE_COLOR/NO_COLOR/TERM.

Re: https://docs.python.org/3/using/cmdline.html#cmdoption-E

Fixing this revealed some tests that failed because of inserted colour codes. For example, in "ZeroDivisionError: division by zero", the parts "ZeroDivisionError", ":" and "division by zero" are all different colours.

For these test cases, I wrapped them with the @force_not_colorized decorator, or, where there were a number of them, I wrapped the test class in the new force_not_colorized_test_class decorator. This was a suggestion from #127223 (comment). And from that PR, I replaced some of the numerous function decorators with the class decorator.

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @hugovk for commit 80422ff 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 13, 2024
@hugovk

This comment was marked as resolved.

@hugovk
Copy link
MemberAuthor

@pablogsal Please could you have a look at this? Thanks!

@brettcannonbrettcannon removed their request for review January 13, 2025 18:21
@serhiy-storchaka
Copy link
Member

Why do we use names without the PYTHON prefix? Are FORCE_COLOR and NO_COLOR the part of some (de-facto?) standard?

@hugovk
Copy link
MemberAuthor

Why do we use names without the PYTHON prefix? Are FORCE_COLOR and NO_COLOR the part of some (de-facto?) standard?

Yes, see https://force-color.org/ and https://no-color.org.

@hugovkhugovk marked this pull request as draft January 20, 2025 16:10
@hugovk
Copy link
MemberAuthor

Please add tests for these environment variables when run Python with the -E option.

Updated to use make_clean_env() from the PyREPL tests (and moved it to test.support for re-use), which removes the PYTHON* env vars to isolate the tests and simulate the -E option.

@hugovkhugovk marked this pull request as ready for review January 21, 2025 14:16
Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM. Although you can use support.force_not_colorized_test_class in few more files.

@hugovkhugovk enabled auto-merge (squash) January 21, 2025 15:54
@hugovk
Copy link
MemberAuthor

Thank you for the reviews!

@hugovkhugovk merged commit 05d12ee into python:mainJan 21, 2025
38 checks passed
@hugovkhugovk deleted the 3.14-fix-colour-env-var-check branch January 21, 2025 16:10
@miss-islington-app
Copy link

Thanks @hugovk for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Sorry, @hugovk, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 05d12eecbde1ace39826320cadf8e673d709b229 3.13 

hugovk added a commit to hugovk/cpython that referenced this pull request Jan 21, 2025
… `PYTHON*` env vars (pythonGH-127877) (cherry picked from commit 05d12ee) Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@bedevere-app
Copy link

GH-129138 is a backport of this pull request to the 3.13 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.13 bugs and security fixes label Jan 21, 2025
hugovk added a commit that referenced this pull request Jan 22, 2025
Sign up for freeto 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.

3 participants

@hugovk@bedevere-bot@serhiy-storchaka