Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-128595: Add test class helper to force no terminal colour#128687

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

hugovk
Copy link
Member

@hugovkhugovk commentedJan 9, 2025
edited
Loading

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.

  • You can disable colorizing for the whole class insetUpClass() instead ofsetUp().
  • Would not it be simpler to implement the core functionality as a generator-based context manager? You can useenterContext() orenterClassContext() with it.
  • You can usetest.support.os_helper.EnvironmentVarGuard() to restore the environment andtest.support.swap_attr() to restore_colorize.can_colorize.

You can also implement this as a mixin instead of patching a method (use asuper() call in an overridden method). I do not say that it would be better, but it is just an alternative which you could have overlooked.

hugovk reacted with thumbs up emoji
Copy link
Member

@vstinnervstinner left a comment

Choose a reason for hiding this comment

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

Oh nice, the new code is more readable, I prefer context managers :-)

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. Much clearer now!

hugovk reacted with hooray emoji
@hugovkhugovkenabled auto-merge (squash)January 13, 2025 11:01
@hugovkhugovk merged commitafb9dc8 intopython:mainJan 13, 2025
40 checks passed
@hugovkhugovk deleted the 3.14-force_not_colorized_test_class branchJanuary 13, 2025 11:05
@miss-islington-app

This comment was marked as outdated.

@miss-islington-app
Copy link

Sorry,@hugovk, I could not cleanly backport this to3.13 due to a conflict.
Please backport usingcherry_picker on command line.

cherry_picker afb9dc887c6e8ae17b6a54c6124399e8bdc82253 3.13

@hugovk
Copy link
MemberAuthor

Thanks for the reviews!

erlend-aasland reacted with heart emoji

hugovk added a commit to hugovk/cpython that referenced this pull requestJan 13, 2025
…lour (pythonGH-128687)(cherry picked from commitafb9dc8)Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@bedevere-app
Copy link

GH-128778 is a backport of this pull request to the3.13 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.13bugs and security fixes labelJan 13, 2025
hugovk added a commit that referenced this pull requestJan 13, 2025
…H-128687) (#128778)Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@vstinnervstinnervstinner left review comments

@serhiy-storchakaserhiy-storchakaserhiy-storchaka approved these changes

@erlend-aaslanderlend-aaslanderlend-aasland approved these changes

@iritkatrieliritkatrielAwaiting requested review from iritkatrieliritkatriel is a code owner

Assignees

@hugovkhugovk

Labels
skip newstestsTests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@hugovk@vstinner@serhiy-storchaka@erlend-aasland

[8]ページ先頭

©2009-2025 Movatter.jp