msg364442 - (view) |
Author: Palak Kumar Jha (palakjha) * |
Date: 2020-03-17 16:19 |
In the PrettyPrinter._format method, since self._dispatch has dict.__repr__ [key] mapped to self._pprint_dict [value] the elif block is not needed. Its work is already being done by the if block above, which searches self._dispatch to fetch the appropriate value.
|
msg375261 - (view) |
Author: Irit Katriel (iritkatriel) * |
Date: 2020-08-12 16:37 |
This change makes a difference in semantics (perhaps a good one) in this case:
import pprint
class MyDict(dict):
def __repr__(self):
return 'I do my own thing'*50
if __name__ == '__main__':
d=MyDict()
for i in range(50):
d['a%s'%i] = i
pprint.pprint(d)
Before the change MyDict.__repr__ is ignored and the dict contents are printed. After the change it prints "I do my own thing" 50 times.
|
msg375270 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2020-08-12 17:38 |
Yes, this code was kept for backward compatibility when the dispatch mapping was added.
|
msg375271 - (view) |
Author: Fred Drake (fdrake) |
Date: 2020-08-12 17:44 |
Adding serhiy.storchaka to nosy list since it looks like he introduced the isinstance check on purpose (see bpo-23741).
Though bpo-23741 asserts that no behavior was changed with the patch applied then, reading through the change leads me to think this did change for cases like the MyDict example from iritkatriel.
The intent of the original code was that MyDict.__repr__ would be used; only the small handful of "known" reprs would be handled specially.
|
msg375279 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2020-08-12 18:48 |
bpo-23741 did not change the behavior. The specified code was added specially to preserve the existing behavior. For other base types the condition was like `issubclass(typ, list) and r is list.__repr__`, but for dict it was just `issubclass(typ, dict)`.
Actually the initial behavior was changed by bad3c88094f43f3bc7dcce22f47b8c2a8dddabcf (no issue number), with adding support of OrderedDict.
|
msg375280 - (view) |
Author: Fred Drake (fdrake) |
Date: 2020-08-12 18:52 |
Ah, good find! It's been so long since the first version of that code, but the implementation was surprisingly nuanced.
|
msg375425 - (view) |
Author: Mariatta (Mariatta) * |
Date: 2020-08-14 18:57 |
@Palak Kumar Jha, you created a PR for this ticket but it seems that your GitHub account is inactive/has been removed. Do you have a new GitHub account? Are you still interested in continuing on this PR?
|
msg375451 - (view) |
Author: Eric V. Smith (eric.smith) * |
Date: 2020-08-15 07:22 |
I went ahead and closed the PR. Either @Palak Kumar Jha or someone else can create a new PR. The suggestions in the original PR should be addresses.
|
msg375453 - (view) |
Author: Irit Katriel (iritkatriel) * |
Date: 2020-08-15 09:02 |
Thanks, I'll create a new PR and credit @Palak Kumar Jha as co-author.
|
msg375465 - (view) |
Author: Irit Katriel (iritkatriel) * |
Date: 2020-08-15 14:05 |
While writing the tests I see that it's even more interesting:
(Pdb) dd = MyDict()
(Pdb) pprint.pformat(dd)
'{}'
(Pdb) pprint.saferepr(dd)
'I do my own thing ....'
|
msg375467 - (view) |
Author: Fred Drake (fdrake) |
Date: 2020-08-15 14:47 |
And that is why the original code was checking not only for the type, but the actual __repr__ method itself.
I think the current behavior is broken.
|
msg375470 - (view) |
Author: Eric V. Smith (eric.smith) * |
Date: 2020-08-15 15:28 |
I realize it might break some corner cases, but I really think we should re-write pprint to use functools.singledispatch. Or if the breakage isn't acceptable, abandon it and create a new module that does use singledispatch. That way it would be easily extensible.
pprint currently works with a dispatch table (_dispatch), it's just hidden.
@fdrake: From the comments: "If you find it useful, thank small children who sleep at night". I assume they're not small anymore!
|
msg375473 - (view) |
Author: Irit Katriel (iritkatriel) * |
Date: 2020-08-15 15:59 |
import pprint
class MyDict(dict):
def __repr__(self):
return '*'*len(dict.__repr__(self))
if __name__ == '__main__':
d=MyDict({})
print('pprint.pformat(d):\n%s' % pprint.pformat(d))
print('pprint.pformat(d, width=1, indent=0):\n%s' % pprint.pformat(d, width=1, indent=0))
Output:
pprint.pformat(d):
**
pprint.pformat(d, width=1, indent=0):
{}
|
msg375485 - (view) |
Author: Irit Katriel (iritkatriel) * |
Date: 2020-08-15 18:18 |
I've created a new PR, with two commits. The first has just tests added, which show the problems and assert that the bug is there. The second has the fix and update to the tests. Let me know what you think.
|
msg375491 - (view) |
Author: Eric V. Smith (eric.smith) * |
Date: 2020-08-15 19:02 |
Only 3.8 - 3.10 would be eligible for this fix. 3.7 is getting only security fixes.
|
msg376117 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2020-08-30 17:29 |
New changeset 582f13786bb75c73d609790967fea03a5b50148a by Irit Katriel in branch 'master':
bpo-39994: Fix pprint handling of dict subclasses that override __repr__ (GH-21892)
https://github.com/python/cpython/commit/582f13786bb75c73d609790967fea03a5b50148a
|
msg376120 - (view) |
Author: Irit Katriel (iritkatriel) * |
Date: 2020-08-30 18:45 |
Thank you Serhiy. This ticket can be closed now.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:59:28 | admin | set | github: 84175 |
2020-08-30 19:46:24 | eric.smith | set | status: open -> closed resolution: fixed stage: patch review -> resolved |
2020-08-30 18:45:10 | iritkatriel | set | messages:
+ msg376120 |
2020-08-30 17:29:57 | serhiy.storchaka | set | messages:
+ msg376117 |
2020-08-15 19:02:47 | eric.smith | set | messages:
+ msg375491 versions:
- Python 3.5, Python 3.6, Python 3.7 |
2020-08-15 18:46:49 | iritkatriel | set | versions:
+ Python 3.5, Python 3.6, Python 3.7, Python 3.10 |
2020-08-15 18:18:54 | iritkatriel | set | messages:
+ msg375485 |
2020-08-15 18:15:18 | iritkatriel | set | pull_requests:
+ pull_request21011 |
2020-08-15 15:59:56 | iritkatriel | set | messages:
+ msg375473 |
2020-08-15 15:28:33 | eric.smith | set | messages:
+ msg375470 |
2020-08-15 14:47:43 | fdrake | set | messages:
+ msg375467 |
2020-08-15 14:05:42 | iritkatriel | set | messages:
+ msg375465 |
2020-08-15 13:34:04 | iritkatriel | set | type: enhancement -> behavior components:
+ Library (Lib) title: Redundant code in pprint module. -> pprint handling of dict subclasses that override __repr__ |
2020-08-15 09:02:59 | iritkatriel | set | messages:
+ msg375453 |
2020-08-15 07:22:53 | eric.smith | set | nosy:
+ eric.smith messages:
+ msg375451
|
2020-08-14 18:57:10 | Mariatta | set | nosy:
+ Mariatta messages:
+ msg375425
|
2020-08-12 18:52:28 | fdrake | set | messages:
+ msg375280 |
2020-08-12 18:48:47 | serhiy.storchaka | set | nosy:
+ rhettinger messages:
+ msg375279
|
2020-08-12 17:44:48 | fdrake | set | messages:
+ msg375271 |
2020-08-12 17:38:53 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka messages:
+ msg375270
|
2020-08-12 16:37:48 | iritkatriel | set | nosy:
+ iritkatriel messages:
+ msg375261
|
2020-03-19 18:43:57 | palakjha | set | nosy:
+ fdrake
versions:
- Python 3.7 |
2020-03-17 16:52:42 | palakjha | set | keywords:
+ patch stage: patch review pull_requests:
+ pull_request18397 |
2020-03-17 16:19:33 | palakjha | create | |