bpo-37376: pprint support for SimpleNamespace by carlbordum · Pull Request #14318 · python/cpython · GitHub

https://bugs.python.org/issue37376

LGTM in general.

Only needed to document this change:

Add the versionchanged directive in the pprint module documentation.
Add a news entry in Misc/NEWS.d (use blurb).
Add an entry in the What's New document.
Add your name in Misc/ACKS if it is not there yet.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

I have made the requested changes; please review again.

Thanks for making the requested changes!

@serhiy-storchaka: please review the changes made to this pull request.

I have made the requested changes; please review again.

Thanks for making the requested changes!

@serhiy-storchaka: please review the changes made to this pull request.

Thanks for working on this! Mostly LGTM. I've left a few comments.

FWIW, code is almost always more readable when limited to one action per line. Among other things, it makes it much easier to understand what's happening (especially when parentheses are involved).

Suggested change

      
                    self.assertEqual(pprint.pformat(types.SimpleNamespace()), "namespace()")
          
                    formatted = pprint.pformat(
          
                        types.SimpleNamespace())
          
                    self.assertEqual(formatted, "namespace()")

Personally I like to separate the 3 parts (setup, actual-thing-to-test, assertions) with blank lines, for further readability, but that's more personal preference:

Suggested change

      
                    self.assertEqual(pprint.pformat(types.SimpleNamespace()), "namespace()")
          
                    ns = types.SimpleNamespace()
          
                    formatted = pprint.pformat(ns)
          
                    self.assertEqual(formatted, "namespace()")

I do not think that adding an empty line after every line of code will make the code more readable. Empty lines should be used for separating logical sections of the code.

This code is consistent with the code in this file and looks clear to me. I do not think that any changes are needed.

I followed most of @ericsnowcurrently's suggestions, but without the empty lines and without textwrap.dedent to be consistent with the rest of the file and I think it has made the test more clear.

As before, I recommend one action per line.

Suggested change

      
                    self.assertEqual(pprint.pformat(n, width=60), """\
          
                    formatted = pprint.pformat(ns, width=60)
          
                    self.assertEqual(formatted, """\

...and separating the key sections of the test.

Suggested change

      
                    self.assertEqual(pprint.pformat(n, width=60), """\
          
                    formatted = pprint.pformat(ns, width=60)
          
                    self.assertEqual(formatted, """\

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

I have made the requested changes; please review again.

@serhiy-storchaka I ended up removing the write = stream.write optimization. If you do think it is worth it in this case, please let me know and I'll bring it back (properly this time, as pointed out by @ericsnowcurrently).

Thanks for making the requested changes!

@ericsnowcurrently, @serhiy-storchaka: please review the changes made to this pull request.

Add a test for a SimpleNamespace subclass.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

I have made the requested changes; please review again.

@serhiy-storchaka ofc it was because of subclassing. Glad you're here to review! :D

Thanks for making the requested changes!

@ericsnowcurrently, @serhiy-storchaka: please review the changes made to this pull request.

Thank you Carl. LGTM, just fix an indentation in docs.

Suggested change

      
              Added support for pretty-printing :class:`types.SimpleNamespace`.
          
               Added support for pretty-printing :class:`types.SimpleNamespace`.

Thanks for working to resolve our various comments!

Other than one small thing, LGTM.

If this check is for the sake of subclasses then I'd recommend being explicit about it:

Suggested change

      
                    if cls_name == "SimpleNamespace":
          
                    if type(object) is _types.SimpleNamespace:

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

I have made the requested changes; please review again.

@ericsnowcurrently I attempted to make it more clear what the class-name-if-statement is about.

Thank you very much to the both of you for the fast, high quality responses. It was a good experience :-)

Thanks for making the requested changes!

@ericsnowcurrently, @serhiy-storchaka: please review the changes made to this pull request.

LGTM. I'm approving this, but also leaving one last comment. :) (feel free to ignore)

Thanks again for doing this.

Thanks for updating this. It's more clear now (as a reader).

And better. Checking the class by comparing the class name to a string is a bit wonky :D Thanks for pointing out, what I was actually doing.

Thanks for adding a comment here. That helps. :) FWIW, the following might be more clear:

# The SimpleNamespace repr has "namespace" instead of the class name, so
# we do the same here.  For subclasses we just use the class name.

Tried improving it, should be clear now

https://bugs.python.org/issue37376

the-knights-who-say-ni added the CLA signed label Jun 23, 2019

bedevere-bot added the awaiting review label Jun 23, 2019

serhiy-storchaka requested changes Jun 23, 2019

View reviewed changes

Comment thread Lib/pprint.py Outdated

bedevere-bot added awaiting changes and removed awaiting review labels Jun 23, 2019

serhiy-storchaka added the type-feature A feature request or enhancement label Jun 23, 2019

carlbordum force-pushed the pprint-simplenamespace branch 2 times, most recently from 8ee8ca1 to 68ff595 Compare June 24, 2019 09:53

bedevere-bot added awaiting change review and removed awaiting changes labels Jun 24, 2019

serhiy-storchaka reviewed Jun 24, 2019

View reviewed changes

Comment thread Doc/library/pprint.rst Outdated

Comment thread Misc/ACKS Outdated

Comment thread Misc/NEWS.d/next/Library/2019-06-24-11-26-34.bpo-37376.SwSUQ4.rst Outdated

carlbordum force-pushed the pprint-simplenamespace branch from 68ff595 to 2bd11f2 Compare June 24, 2019 14:56

serhiy-storchaka reviewed Jun 24, 2019

View reviewed changes

Comment thread Doc/library/pprint.rst Outdated

Comment thread Doc/library/pprint.rst Outdated

ericsnowcurrently requested changes Jun 24, 2019

View reviewed changes

bedevere-bot removed the awaiting change review label Jun 24, 2019

bedevere-bot added the awaiting changes label Jun 24, 2019

carlbordum force-pushed the pprint-simplenamespace branch from 2bd11f2 to 7d3fbba Compare June 25, 2019 09:19

bedevere-bot added awaiting change review and removed awaiting changes labels Jun 25, 2019

serhiy-storchaka requested changes Jun 25, 2019

View reviewed changes

bedevere-bot removed the awaiting change review label Jun 25, 2019

carlbordum force-pushed the pprint-simplenamespace branch from 7d3fbba to b701a7a Compare June 25, 2019 18:42

bedevere-bot added awaiting change review and removed awaiting changes labels Jun 25, 2019

serhiy-storchaka approved these changes Jun 25, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jun 25, 2019

ericsnowcurrently requested changes Jun 25, 2019

View reviewed changes

bedevere-bot removed the awaiting merge label Jun 25, 2019

bedevere-bot added the awaiting changes label Jun 25, 2019

carlbordum force-pushed the pprint-simplenamespace branch 2 times, most recently from a584309 to 49fa618 Compare June 26, 2019 16:40

bedevere-bot added awaiting change review and removed awaiting changes labels Jun 26, 2019

ericsnowcurrently approved these changes Jun 26, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jun 26, 2019

bpo-37376: pprint support for SimpleNamespace

8867ef7

carlbordum force-pushed the pprint-simplenamespace branch from 49fa618 to 8867ef7 Compare June 26, 2019 20:39

ericsnowcurrently added the 🤖 automerge label Jun 26, 2019

miss-islington merged commit 06a8916 into python:master Jun 26, 2019

bedevere-bot removed the awaiting merge label Jun 26, 2019

lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019

bpo-37376: pprint support for SimpleNamespace (pythonGH-14318 )

6d4192d

https://bugs.python.org/issue37376

DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020

bpo-37376: pprint support for SimpleNamespace (pythonGH-14318 )

d3a73a4

https://bugs.python.org/issue37376

LewisGaul mentioned this pull request Jan 31, 2021

bpo-43080: pprint for dataclass instances #24389

Merged

	self.assertEqual(pprint.pformat(n, width=60), """\
	formatted = pprint.pformat(ns, width=60)
	self.assertEqual(formatted, """\

	Added support for pretty-printing :class:`types.SimpleNamespace`.
	Added support for pretty-printing :class:`types.SimpleNamespace`.

	if cls_name == "SimpleNamespace":
	if type(object) is _types.SimpleNamespace:

bpo-37376: pprint support for SimpleNamespace by carlbordum · Pull Request #14318 · python/cpython · GitHub

Repository navigation

bpo-37376: pprint support for SimpleNamespace - #14318

bpo-37376: pprint support for SimpleNamespace#14318miss-islington merged 1 commit intopython:masterpython/cpython:masterfrom carlbordum:pprint-simplenamespacecarlbordum/cpython:pprint-simplenamespaceCopy head branch name to clipboard

Conversation

carlbordum commented Jun 23, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-bot commented Jun 23, 2019

Uh oh!

carlbordum commented Jun 24, 2019

Uh oh!

bedevere-bot commented Jun 24, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlbordum commented Jun 24, 2019

Uh oh!

bedevere-bot commented Jun 24, 2019

Uh oh!

Uh oh!

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Jun 24, 2019

Uh oh!

carlbordum commented Jun 25, 2019

Uh oh!

bedevere-bot commented Jun 25, 2019

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jun 25, 2019

Uh oh!

carlbordum commented Jun 25, 2019

Uh oh!

bedevere-bot commented Jun 25, 2019

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jun 25, 2019

Uh oh!

carlbordum commented Jun 26, 2019

Uh oh!

bedevere-bot commented Jun 26, 2019

Uh oh!

ericsnowcurrently left a comment

bpo-37376: pprint support for SimpleNamespace#14318
miss-islington merged 1 commit into
python:masterfrom
carlbordum:pprint-simplenamespace

carlbordum commented Jun 23, 2019 •
edited by bedevere-bot
Loading

ericsnowcurrently Jun 25, 2019 •
edited
Loading