fix for 1712742: corrects pprint's handling of 'depth' #44929
Comments
|
The patch is really simple. Just change the way "maxlevels" is checked. I couldn't find a way to have a test case for this bug. |
|
File Added: pprint_test.py |
|
Your patch looks good and worked fine. Wouldn't it be nice to add a test case for this problem to test_pprint.py? Index: Lib/test/test_pprint.py --- Lib/test/test_pprint.py (revision 55144)
+++ Lib/test/test_pprint.py (working copy)
@@ -195,7 +195,22 @@
others.should.not.be: like.this}"""
self.assertEqual(DottedPrettyPrinter().pformat(o), exp)
+ def test_depth(self):
+ nested_tuple = (1, (2, (3, (4, (5, 6)))))
+ nested_dict = {1: {2: {3: {4: {5: {6: 6}}}}}}
+ nested_list = [1, [2, [3, [4, [5, [6, []]]]]]]
+ self.assertEqual(pprint.pformat(nested_tuple), str(nested_tuple))
+ self.assertEqual(pprint.pformat(nested_dict), str(nested_dict))
+ self.assertEqual(pprint.pformat(nested_list), str(nested_list))
+
+ lv1_tuple = '(1, (...))'
+ lv1_dict = '{1: {...}}'
+ lv1_list = '[1, [...]]'
+ self.assertEqual(pprint.pformat(nested_tuple, depth=1), lv1_tuple)
+ self.assertEqual(pprint.pformat(nested_dict, depth=1), lv1_dict)
+ self.assertEqual(pprint.pformat(nested_list, depth=1), lv1_list)
+ |
|
josm, thanks for the test case. I incorporated it into the patch. BTW, I changed "str" to "repr" as I think "repr" is closer to what pformat does. |
|
I notice in the doc an example which doesn't work with this patch. It still prints one level too deep. The doc seems correct to me, but I don't have strong feelings any way. The attached patch makes the doc example work as expected. The doc should really be updated with an example more like: >>> pp = pprint.PrettyPrinter(depth=6)
>>> pp.pprint((1, (2, (3, (4, (5, (6, 7)))))))
(1, (2, (3, (4, (5, (...))))))
>>> pp = pprint.PrettyPrinter(depth=1)
>>> pp.pprint(1)
1
>>> pp.pprint([1])
[...]The updated patch causes the new tests to fail. Could you update the test/code/doc to all be consistent? |
|
Hi Neal, I assume you are referring to the example >>> import parser
>>> tup = parser.ast2tuple(
... parser.suite(open('pprint.py').read()))[1][1][1]
>>> pp = pprint.PrettyPrinter(depth=6)
>>> pp.pprint(tup)in the document. Is that correct? I ran this example with and without my patch. Without the update, the example printed 7 levels which is one level too deep. With the patch, it printed 6 levels, which seems correct to me. # without patch. prints 7 levels. # with patch. prints 6 levels. I am attaching the file testdoc.py which contains the doc example. I just wanted to confirm that this is what you are referring to. File Added: testdoc.py |
|
Yes, that's the example I'm referring to. In the doc, it shows 5 numbers and one ... for 6. Without your patch it shows 7 numbers and with your patch it shows 6 numbers. So even with your patch the doc and code don't agree (they are off by one, rather than 2 as is currently the case). |
|
I agree. The code and the doc should be consistent. New test would be Index: Lib/test/test_pprint.py --- Lib/test/test_pprint.py (revision 55223)
+++ Lib/test/test_pprint.py (working copy)
@@ -195,7 +195,27 @@
others.should.not.be: like.this}"""
self.assertEqual(DottedPrettyPrinter().pformat(o), exp)
+ def test_depth(self):
+ nested_tuple = (1, (2, (3, (4, (5, 6)))))
+ nested_dict = {1: {2: {3: {4: {5: {6: 6}}}}}}
+ nested_list = [1, [2, [3, [4, [5, [6, []]]]]]]
+ self.assertEqual(pprint.pformat(nested_tuple), repr(nested_tuple))
+ self.assertEqual(pprint.pformat(nested_dict), repr(nested_dict))
+ self.assertEqual(pprint.pformat(nested_list), repr(nested_list))
+
+ result_tuple = {'lv1': '(...)',
+ 'lv2': '(1, (...))'}
+ result_dict = {'lv1': '{...}',
+ 'lv2': '{1: {...}}'}
+ result_list = {'lv1': '[...]',
+ 'lv2': '[1, [...]]'}
+ for i in [1, 2]:
+ key = 'lv' + `i`
+ self.assertEqual(pprint.pformat(nested_tuple, depth=i), result_tuple[key])
+ self.assertEqual(pprint.pformat(nested_dict, depth=i), result_dict[key])
+ self.assertEqual(pprint.pformat(nested_list, depth=i), result_list[key])
+
class DottedPrettyPrinter(pprint.PrettyPrinter):
def format(self, object, context, maxlevels, level): |
|
josm, I don't think there is need for a new test case. The one in the patch correctly tests the code change. As Neal pointed out, the doc is wrong to begin with. I updated the patch with the doc change. Note that I replaced the current example with a very simple one. I believe the current one is overly complicated for this purpose. Comments are welcome, of course. |
|
File Added: pprint.patch |
|
That test case was for pp2.diff. I just thought if changing the doc itself would cause some problem, Don't we have to get someone's approval to change the spec? |
|
josm> I just thought if changing the doc itself would cause Not if the doc is wrong. josm> Don't we have to get someone's approval to change the We are not changing the spec here. |
|
Yes, the doc seems to be wrong, and I agree with you I think I can call this as 'a spec change'. Don't we need a consensus among community? No one cares so much how pprint works? |
|
josm, merely changing (fixing) the handling of "depth" parameter is not a spec change. The patch makes pprint behave in a way that is intuitive from the meaning of "depth". |
|
What is intuitive is a matter of taste. |
|
josm, please feel free to go to python-dev if you think community input is required. I personally don't think that it is warranted. |
|
Neal, is there anything else you want me to do for the patch? |
|
+1 on the patch (test, code and doc). In particular, if depth == 0 is not allowed, I think the patched behaviour is the expected one, so this is actually a bug fix rather than a change in the module's semantic. |
|
Rodrigo, which patch are you giving +1? Sorry Raghu, I'm getting behind. :-( I'll try to take a look at this. If everything is consistent (docs, code, and tests) and has sane semantics, there shouldn't be anything else necessary. I just need to get the time to apply. Or someone else with commit privileges could apply this as well. If I don't get to this within a week or so, feel free to ping me via mail. |
|
Neal: sorry, I understood pprint.patch was the "current", valid patch, and the object of the most recent discussion. This is the one I was referring to. |
|
Neal, can you please apply the patch? - Thanks. |
|
I was just searching for all my issues and found this one. Can some one |
|
The test suite doesn't pass any longer when the patch is applied. |
|
It seems that somewhere along the road between revision 55144 (where the I've attached a patch that fixes this, along with the fixes this issue BTW, doctesting Doc/library/pprint.rst with optionflags=doctest.ELLIPSIS |
|
Okay, thank you all! Committed patch as r63164. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: