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-135074: Fix exception messages in test.support module#135076

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

danielhollas
Copy link
Contributor

@danielhollasdanielhollas commentedJun 3, 2025
edited by bedevere-appbot
Loading

Fixes#135074.

The issue also exists on 3.13 and 3.14 branches, not sure if changes totest.support are backported? (also not sure if blurb is needed here).

@danielhollasdanielhollas changed the titlegh-1350745: Fix exception messages in test.support modulegh-135074: Fix exception messages in test.support moduleJun 3, 2025
@ZeroIntensityZeroIntensity added the testsTests in the Lib/test dir labelJun 3, 2025
@skirpichevskirpichev added skip news needs backport to 3.13bugs and security fixes needs backport to 3.14bugs and security fixes labelsJun 3, 2025
Copy link
Contributor

@skirpichevskirpichev left a comment

Choose a reason for hiding this comment

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

Though, this is not user-visible change and I suggest removing the news entry.

danielhollas reacted with thumbs up emoji
This reverts commit44f80c7.
@danielhollas
Copy link
ContributorAuthor

danielhollas commentedJun 3, 2025
edited
Loading

Just for my own curiosity, what's the difference between "awaiting review" and "awaiting core review"?

(btw: these labels don't have descriptions on GitHub, e.g. herehttps://github.com/python/cpython/labels)

Though, this is not user-visible change and I suggest removing the news entry.

Okay, done.

@ZeroIntensity
Copy link
Member

Just for my own curiosity, what's the difference between "awaiting review" and "awaiting core review"?

awaiting review is awaiting a review from anyone.awaiting core review is awaiting a review from acore developer.

danielhollas reacted with thumbs up emoji

Copy link
Member

@ZeroIntensityZeroIntensity left a comment

Choose a reason for hiding this comment

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

LGTM as well. In the future, let's avoid changes to private modules.

@danielhollas
Copy link
ContributorAuthor

In the future, let's avoid changes to private modules.

@ZeroIntensity sorry, can you explain what you mean by this?

@ZeroIntensity
Copy link
Member

ZeroIntensity commentedJun 3, 2025
edited
Loading

test isn't a module that isn't shipped to Python users. So, fixing hypothetical bugs in it isn't really solving an actual problem. We might as well go with this one because it's already here, but don't go searching private modules for problems, unless they're accessible by a public API.

@skirpichev
Copy link
Contributor

So, fixing hypothetical bugs in it isn't really solving an actual problem.

I think this time it's a real issue;-) Second test helper used a lot.

@ZeroIntensity
Copy link
Member

Yeah, but it doesn't come up in practice, really.

@danielhollas
Copy link
ContributorAuthor

So, fixing hypothetical bugs in it isn't really solving an actual problem.

Yeah, but it doesn't come up in practice, really.

I am very confused by these arguments. The code that I fixed was clearly not doing what was intended (nothing hypothetical about that), the fact that it is in a rarely travelled error path seems irrelevant. This is not a pointless refactor / style PR, I understand these are discouraged here.

but don't go searching private modules for problems, unless they're accessible by a public API.

Are we saying people should not submit bugfixes for internal python tooling/modules? "going searching for problems" seems rather dismissive of somebody spotting a bug. Also note that this PR was just a side-product of discovering the exact same issue in stdlib in#135069.

Sorry, I don't want to argue and get all defensive here and I understand the desire to reduce unnecessary churn, but also please understand that this was quite discouraging for me to hear, and I assume would be for other well-meaning contributors.

@ZeroIntensity
Copy link
Member

Sorry, I don't want to argue and get all defensive here and I understand the desire to reduce unnecessary churn, but also please understand that this was quite discouraging for me to hear, and I assume would be for other well-meaning contributors.

My bad! That wasn't my intention at all. I was trying to give advice for future PRs--basically, try to avoid churn. You're good to do this for the standard library, it's just not helpful to spend time fixing things that nobody will really benefit from. It's a bug, yes, it's just on the very bottom of the priority list. Does that make more sense?

danielhollas reacted with heart emoji

@danielhollas
Copy link
ContributorAuthor

All good, thanks! 👍

@encukouencukou merged commitbc00ce9 intopython:mainJun 4, 2025
38 checks passed
@miss-islington-app
Copy link

Thanks@danielhollas for the PR, and@encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestJun 4, 2025
…nGH-135076)(cherry picked from commitbc00ce9)Co-authored-by: Daniel Hollas <daniel.hollas@bristol.ac.uk>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestJun 4, 2025
…nGH-135076)(cherry picked from commitbc00ce9)Co-authored-by: Daniel Hollas <daniel.hollas@bristol.ac.uk>
@bedevere-app
Copy link

GH-135129 is a backport of this pull request to the3.14 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.14bugs and security fixes labelJun 4, 2025
@bedevere-app
Copy link

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

@bedevere-appbedevere-appbot removed the needs backport to 3.13bugs and security fixes labelJun 4, 2025
@encukou
Copy link
Member

not sure if changes to test.support are backported?

Yes, but the reason is not obvious: avoiding merge conflicts when an important fix will need to be backported.

danielhollas reacted with thumbs up emoji

@danielhollasdanielhollas deleted the test-support-fix-fstrings branchJune 4, 2025 13:11
encukou pushed a commit that referenced this pull requestJun 4, 2025
…35076) (GH-135130)(cherry picked from commitbc00ce9)Co-authored-by: Daniel Hollas <daniel.hollas@bristol.ac.uk>
encukou pushed a commit that referenced this pull requestJun 4, 2025
…35076) (GH-135129)(cherry picked from commitbc00ce9)Co-authored-by: Daniel Hollas <daniel.hollas@bristol.ac.uk>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@skirpichevskirpichevskirpichev approved these changes

@ZeroIntensityZeroIntensityZeroIntensity approved these changes

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

Successfully merging this pull request may close these issues.

Unexpanded f-strings in Lib/test/support/__init__.py exceptions
4 participants
@danielhollas@ZeroIntensity@skirpichev@encukou

[8]ページ先頭

©2009-2025 Movatter.jp