-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-92546: Fix invalid call in pprint executed as a script #92560
Conversation
Need backporting to 3.11 and 3.10 only. 3.9 runs flawlessly. |
It would be good to add a test for this so we don't get out of sync in the future. I don't think we need to call it through |
Please also request a review from the dev who introduced the bug. This is an important part of the feed back loop. |
@@ -643,7 +643,7 @@ def _perfcheck(object=None): | |||
object = [("string", (1, 2), [3, 4], {5: 6, 7: 8})] * 100000 | |||
p = PrettyPrinter() | |||
t1 = time.perf_counter() | |||
p._safe_repr(object, {}, None, 0, True) | |||
p._safe_repr(object, {}, None, 0) |
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.
Thank for reporting. We typically commit a bugfix together with a unit test that will prevent the bug reoccurring. I'm not sure though whether _perfcheck is a feature that needs to be fixed (is it user facing?) or dead code that needs to be removed or a test that needs to move to the test module.
Any idea what it's for?
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.
It isn't user facing and it isn't dead. We sometime have code the is just for us in a main section. For example, the random module also has some performance measurement code and example distributions — we use that sometimes while maintaining the module.
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.
If it’s useful then it needs a test.
(And it would probably be even more useful as a pyperformance benchmark, where it would be tracked on a regular basis.)
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.
I'd write a sanity test which verifies that $ python -m pprint
works. More unit tests could then be added in the future if needed, but a simple sanity test is enough for now.
From what I can tell, that is what didn't work for @ArturKhuziakhmetov when they opened #92546, and it makes sense to test. It is surely user-facing, and should work.
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.
Update: I just noticed I missed some of @rhettinger's comments. Since this is undocumented functionality, refraining from testing it may make sense. However, seeing broken code shipped in production builds raises a red flag for me.
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.
But the production code is really the functions that people import from the pprint module, not the unofficial script functionality.
@iritkatriel : it doesn't seem very useful to me, and as far as I can tell it's not documented. I suggest either adding tests for it or deleting it, with a preference to delete it. |
Misc/NEWS.d/next/Library/2022-05-09-17-31-30.gh-issue-92546.1hbeQQ.rst
Outdated
Show resolved
Hide resolved
Closing in favor of python/pyperformance#222 and gh-94613. |
This PR couples with python/pyperformance#222 and supersedes #92560. Inspired by #93096 (comment). Automerge-Triggered-By: GH:ericsnowcurrently
Before the fix:
After the fix:
Issue: gh-92546.