classification
Title: fix for 1712742: corrects pprint's handling of 'depth'
Type: Stage:
Components: Library (Lib) Versions: Python 2.6
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: nnorwitz Nosy List: draghuram, georg.brandl, josm, nnorwitz, rbp
Priority: normal Keywords: easy, patch

Created on 2007-05-04 19:13 by draghuram, last changed 2008-05-12 16:27 by georg.brandl. This issue is now closed.

Files
File name Uploaded Description Edit
pprint_test.py draghuram, 2007-05-04 19:15 script to test pprint with recirsive objects
pp2.diff nnorwitz, 2007-05-09 06:56 with depth per request in bug report
pprint.patch draghuram, 2007-05-10 14:13
pprint-r63129.patch rbp, 2008-05-12 04:09
Messages (25)
msg52576 - (view) Author: Raghuram Devarakonda (draghuram) (Python triager) Date: 2007-05-04 19:13
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.

msg52577 - (view) Author: Raghuram Devarakonda (draghuram) (Python triager) Date: 2007-05-04 19:15
File Added: pprint_test.py
msg52578 - (view) Author: John Smith (josm) Date: 2007-05-05 09:02
Your patch looks good and worked fine.

Wouldn't it be nice to add a test case for this problem to test_pprint.py?
The following is just draft patch to add the test case.

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)
+
msg52579 - (view) Author: Raghuram Devarakonda (draghuram) (Python triager) Date: 2007-05-07 14:49

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.
File Added: pprint.patch
msg52580 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2007-05-09 06:56
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?
File Added: pp2.diff
msg52581 - (view) Author: Raghuram Devarakonda (draghuram) (Python triager) Date: 2007-05-09 14:44

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.
$ ../python/python testdoc.py 
(268, (269, (326, (303, (304, (305, (306, (...))))))))

# with patch. prints 6 levels.
$ ../python/python testdoc.py 
(268, (269, (326, (303, (304, (305, (...)))))))

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
msg52582 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2007-05-10 06:18
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).
msg52583 - (view) Author: John Smith (josm) Date: 2007-05-10 11:56
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):
msg52584 - (view) Author: Raghuram Devarakonda (draghuram) (Python triager) Date: 2007-05-10 14:13

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. 
msg52585 - (view) Author: Raghuram Devarakonda (draghuram) (Python triager) Date: 2007-05-10 14:13
File Added: pprint.patch
msg52586 - (view) Author: John Smith (josm) Date: 2007-05-10 21:37
That test case was for pp2.diff.

I just thought if changing the doc itself would cause some problem,
we have to fix the code to work the way the doc says.

Don't we have to get someone's approval to change the spec?

msg52587 - (view) Author: Raghuram Devarakonda (draghuram) (Python triager) Date: 2007-05-10 21:41

josm> I just thought if changing the doc itself would cause 
josm> some problem, we have to fix the code to work the way 
josm> the doc says.

Not if the doc is wrong. 

josm> Don't we have to get someone's approval to change the 
josm> spec?

We are not changing the spec here.

msg52588 - (view) Author: John Smith (josm) Date: 2007-05-12 14:09
 Yes, the doc seems to be wrong, and I agree with you 
changing wrong doc is always right thing to do, 
but this patch changed the way how pprint handles 'depth'. 

 I think I can call this as 'a spec change'.
changing some existent module's behavior is 
something that needs to be done carefully.

Don't we need a consensus among community?

No one cares so much how pprint works?
could be ...
msg52589 - (view) Author: Raghuram Devarakonda (draghuram) (Python triager) Date: 2007-05-13 11:56

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".
msg52590 - (view) Author: John Smith (josm) Date: 2007-05-13 12:14
What is intuitive is a matter of taste.
Some people like to count from zero, like many programmers do.
msg52591 - (view) Author: Raghuram Devarakonda (draghuram) (Python triager) Date: 2007-05-13 15:09
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.
msg52592 - (view) Author: Raghuram Devarakonda (draghuram) (Python triager) Date: 2007-05-18 21:16
Neal, is there anything else you want me to do for the patch?
msg52593 - (view) Author: Rodrigo Bernardo Pimentel (errebepe) Date: 2007-05-31 03:06
+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. 
msg52594 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2007-06-01 06:48
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.
msg52595 - (view) Author: Rodrigo Bernardo Pimentel (errebepe) Date: 2007-06-01 14:03
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.
msg52596 - (view) Author: Raghuram Devarakonda (draghuram) (Python triager) Date: 2007-06-29 14:12
Neal, can you please apply the patch? - Thanks.
msg56292 - (view) Author: Raghuram Devarakonda (draghuram) (Python triager) Date: 2007-10-09 19:58
I was just searching for all my issues and found this one. Can some one
please apply pprint.patch? Thanks.
msg66686 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2008-05-11 22:11
The test suite doesn't pass any longer when the patch is applied.
msg66706 - (view) Author: Rodrigo Bernardo Pimentel (rbp) (Python committer) Date: 2008-05-12 04:09
It seems that somewhere along the road between revision 55144 (where the
first patch was generated) and current trunk (revision 63129),
PrettyPrinter._format has stopped handling depth!

I've attached a patch that fixes this, along with the fixes this issue
originally proposed (including the tests and documentation updates).
With this patch, unit tests and the documentation's doctests all pass.

BTW, doctesting Doc/library/pprint.rst with optionflags=doctest.ELLIPSIS
erroneously interprets pprint's '...' (indicating depth exceeded) as
doctest ellipses! Testing without ELLIPSIS gives an error on output with
something like "[<Recursion on list with id=...>]", testing with it
hides errors. Ugh!
msg66736 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2008-05-12 16:27
Okay, thank you all! Committed patch as r63164.
History
Date User Action Args
2008-05-12 16:27:06georg.brandlsetstatus: open -> closed
resolution: accepted
messages: + msg66736
2008-05-12 16:25:52georg.brandllinkissue1712742 superseder
2008-05-12 04:09:26rbpsetfiles: + pprint-r63129.patch
nosy: + rbp, - errebepe
messages: + msg66706
2008-05-11 22:11:52georg.brandlsetnosy: + georg.brandl
messages: + msg66686
2008-01-14 18:19:34christian.heimessetkeywords: + easy
2007-10-09 19:58:03draghuramsetmessages: + msg56292
2007-05-04 19:13:59draghuramcreate