Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.1k
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
Conversation
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.
Though, this is not user-visible change and I suggest removing the news entry.
This reverts commit44f80c7.
danielhollas commentedJun 3, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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)
Okay, done. |
|
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 as well. In the future, let's avoid changes to private modules.
@ZeroIntensity sorry, can you explain what you mean by this? |
ZeroIntensity commentedJun 3, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
I think this time it's a real issue;-) Second test helper used a lot. |
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.
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. |
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? |
All good, thanks! 👍 |
bc00ce9
intopython:mainUh oh!
There was an error while loading.Please reload this page.
Thanks@danielhollas for the PR, and@encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14. |
…nGH-135076)(cherry picked from commitbc00ce9)Co-authored-by: Daniel Hollas <daniel.hollas@bristol.ac.uk>
…nGH-135076)(cherry picked from commitbc00ce9)Co-authored-by: Daniel Hollas <daniel.hollas@bristol.ac.uk>
GH-135129 is a backport of this pull request to the3.14 branch. |
GH-135130 is a backport of this pull request to the3.13 branch. |
Yes, but the reason is not obvious: avoiding merge conflicts when an important fix will need to be backported. |
Uh oh!
There was an error while loading.Please reload this page.
Fixes#135074.
The issue also exists on 3.13 and 3.14 branches, not sure if changes to
test.support
are backported? (also not sure if blurb is needed here).