GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-127873: Only check `sys.flags.ignore_environment` for `PYTHON*` env vars by hugovk · Pull Request #127877 · python/cpython

Conversation

@hugovk

@hugovk hugovk commented Dec 12, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

  • Issue: can_colorize() ignores FORCE_COLOR/NO_COLOR/TERM when -E is set #127873

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
hugovk added 5 commits December 12, 2024 18:34
@hugovk
Only check sys.flags.ignore_environment before PYTHON\* env vars
71df920
@hugovk
Add test class helper to force no terminal colour
b664cd4
@hugovk
Refactor
adae1cb
@hugovk
Replace force_not_colorized with force_not_colorized_test_class
41c3576
@hugovk
Fix tests when run with FORCE_COLOR=1
80422ff
@hugovk hugovk added the needs backport to 3.13 only security fixes label Dec 12, 2024
@hugovk
hugovk requested review from brettcannon, ericsnowcurrently, ezio-melotti, iritkatriel, markshannon, ncoghlan and warsaw as code owners December 12, 2024 16:55
@bedevere-app bedevere-app Bot mentioned this pull request Dec 12, 2024
can_colorize() ignores FORCE_COLOR/NO_COLOR/TERM when -E is set #127873
Closed
@bedevere-app bedevere-app Bot added the awaiting core review label Dec 12, 2024
@hugovk hugovk added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 13, 2024
@bedevere-bot

bedevere-bot commented Dec 13, 2024

Copy link
Copy Markdown

🤖 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-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.

Sign in to view
@hugovk
Merge branch 'main' into 3.14-fix-colour-env-var-check
6980f3e
@hugovk

hugovk commented Dec 17, 2024

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@hugovk
Merge remote-tracking branch 'upstream/main' into 3.14-fix-colour-env…
d3f9e10 
…-var-check
This was referenced Jan 4, 2025
gh-128595: Default to stdout isatty for colour detection instead of stderr #128498
Merged
gh-128595: Add test class helper to force no terminal colour #128687
Merged
hugovk added 2 commits January 13, 2025 14:22
@hugovk
Merge branch 'main' into 3.14-fix-colour-env-var-check
9e6eb93
@hugovk
Fix merge conflict resolution
76f656e
@brettcannon
brettcannon removed their request for review January 13, 2025 18:21
@serhiy-storchaka

serhiy-storchaka commented Jan 20, 2025

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka reviewed Jan 20, 2025
View reviewed changes
Comment thread Lib/test/test_capi/test_misc.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_capi/test_misc.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@hugovk

hugovk commented Jan 20, 2025

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@hugovk
Revert more import sorting
cd88069
@hugovk
hugovk marked this pull request as draft January 20, 2025 16:10
@bedevere-app bedevere-app Bot removed the awaiting core review label Jan 20, 2025
hugovk added 2 commits January 21, 2025 13:54
@hugovk
Merge remote-tracking branch 'upstream/main' into 3.14-fix-colour-env…
37e0bbd 
…-var-check
@hugovk
Remove PYTHON* env vars to isolate colour tests
4be7293
@hugovk

hugovk commented Jan 21, 2025

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@hugovk
hugovk marked this pull request as ready for review January 21, 2025 14:16
@hugovk
hugovk requested review from AA-Turner, ambv, lysnikolaou and pablogsal as code owners January 21, 2025 14:16
@bedevere-app bedevere-app Bot added the awaiting core review label Jan 21, 2025
serhiy-storchaka
serhiy-storchaka approved these changes Jan 21, 2025
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Jan 21, 2025
@hugovk
Revert more imports
302756c
@hugovk
hugovk enabled auto-merge (squash) January 21, 2025 15:54
@hugovk

hugovk commented Jan 21, 2025

Copy link
Copy Markdown
Member Author

Thank you for the reviews!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@hugovk
hugovk merged commit 05d12ee into python:main Jan 21, 2025
@hugovk
hugovk deleted the 3.14-fix-colour-env-var-check branch January 21, 2025 16:10
@miss-islington-app

miss-islington-app Bot commented Jan 21, 2025

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the awaiting merge label Jan 21, 2025
@miss-islington-app

miss-islington-app Bot commented Jan 21, 2025

Copy link
Copy Markdown

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington-app miss-islington-app Bot assigned hugovk Jan 21, 2025
hugovk added a commit to hugovk/cpython that referenced this pull request Jan 21, 2025
@hugovk
[3.13] pythongh-127873: Only check sys.flags.ignore_environment for…
5c86cc4 
… `PYTHON*` env vars (pythonGH-127877)

(cherry picked from commit 05d12ee)

Co-authored-by: Hugo van Kemenade <[email protected]>
@bedevere-app

bedevere-app Bot commented Jan 21, 2025

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.13 only security fixes label Jan 21, 2025
hugovk added a commit that referenced this pull request Jan 22, 2025
@hugovk
[3.13] gh-127873: Only check sys.flags.ignore_environment for `PYTH…
cc3dc8a 
…ON*` env vars (GH-127877) (#129138)
This was referenced Jan 22, 2025
Add colour to GitHub Actions output #129196
Merged
Improve tests for _colorize.can_colorize() #129234
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@ncoghlan ncoghlan Awaiting requested review from ncoghlan ncoghlan is a code owner
@warsaw warsaw Awaiting requested review from warsaw warsaw is a code owner
@iritkatriel iritkatriel Awaiting requested review from iritkatriel iritkatriel is a code owner
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner
@ezio-melotti ezio-melotti Awaiting requested review from ezio-melotti ezio-melotti is a code owner
@pablogsal pablogsal Awaiting requested review from pablogsal pablogsal is a code owner
@lysnikolaou lysnikolaou Awaiting requested review from lysnikolaou lysnikolaou is a code owner
@ambv ambv Awaiting requested review from ambv ambv is a code owner
@AA-Turner AA-Turner Awaiting requested review from AA-Turner AA-Turner is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@hugovk hugovk

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@hugovk @bedevere-bot @serhiy-storchaka
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.