◐ Shell
reader mode source ↗
Skip to content

bpo-37376: pprint support for SimpleNamespace#14318

Merged
miss-islington merged 1 commit into
python:masterfrom
carlbordum:pprint-simplenamespace
Jun 26, 2019
Merged

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

Conversation

@carlbordum

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

Copy link
Copy Markdown
Contributor

serhiy-storchaka
serhiy-storchaka requested changes Jun 23, 2019
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

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.

@bedevere-bot

bedevere-bot commented Jun 23, 2019

Copy link
Copy Markdown

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.

@serhiy-storchaka serhiy-storchaka added the type-feature A feature request or enhancement label Jun 23, 2019
@carlbordum carlbordum force-pushed the pprint-simplenamespace branch 2 times, most recently from 8ee8ca1 to 68ff595 Compare June 24, 2019 09:53
@carlbordum

carlbordum commented Jun 24, 2019

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again.

@bedevere-bot

bedevere-bot commented Jun 24, 2019

Copy link
Copy Markdown

Thanks for making the requested changes!

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

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

carlbordum commented Jun 24, 2019

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again.

@bedevere-bot

bedevere-bot commented Jun 24, 2019

Copy link
Copy Markdown

Thanks for making the requested changes!

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

ericsnowcurrently
ericsnowcurrently requested changes Jun 24, 2019
View reviewed changes

@ericsnowcurrently ericsnowcurrently left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

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

1 hidden conversation Load more…
@bedevere-bot

bedevere-bot commented Jun 24, 2019

Copy link
Copy Markdown

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.

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

carlbordum commented Jun 25, 2019

Copy link
Copy Markdown
Contributor Author

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).

@bedevere-bot

bedevere-bot commented Jun 25, 2019

Copy link
Copy Markdown

Thanks for making the requested changes!

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

serhiy-storchaka
serhiy-storchaka requested changes Jun 25, 2019
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Add a test for a SimpleNamespace subclass.

@bedevere-bot

bedevere-bot commented Jun 25, 2019

Copy link
Copy Markdown

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.

16 hidden items Load more…
@carlbordum carlbordum force-pushed the pprint-simplenamespace branch from 7d3fbba to b701a7a Compare June 25, 2019 18:42
@carlbordum

carlbordum commented Jun 25, 2019

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again.

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

@bedevere-bot

bedevere-bot commented Jun 25, 2019

Copy link
Copy Markdown

Thanks for making the requested changes!

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

serhiy-storchaka
serhiy-storchaka approved these changes Jun 25, 2019
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

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

ericsnowcurrently
ericsnowcurrently requested changes Jun 25, 2019
View reviewed changes

@ericsnowcurrently ericsnowcurrently left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Thanks for working to resolve our various comments!

Other than one small thing, LGTM.

@bedevere-bot

bedevere-bot commented Jun 25, 2019

Copy link
Copy Markdown

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.

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

carlbordum commented Jun 26, 2019

Copy link
Copy Markdown
Contributor Author

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 :-)

@bedevere-bot

bedevere-bot commented Jun 26, 2019

Copy link
Copy Markdown

Thanks for making the requested changes!

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

ericsnowcurrently
ericsnowcurrently approved these changes Jun 26, 2019
View reviewed changes

@ericsnowcurrently ericsnowcurrently left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

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

Thanks again for doing this.

@carlbordum carlbordum force-pushed the pprint-simplenamespace branch from 49fa618 to 8867ef7 Compare June 26, 2019 20:39
@miss-islington miss-islington merged commit 06a8916 into python:master Jun 26, 2019
lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
@carlbordum @lisroach
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
@carlbordum @DinoV
bpo-37376: pprint support for SimpleNamespace (pythonGH-14318) …
d3a73a4 
https://bugs.python.org/issue37376
@LewisGaul LewisGaul mentioned this pull request Jan 31, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericsnowcurrently ericsnowcurrently ericsnowcurrently approved these changes

@serhiy-storchaka serhiy-storchaka

Assignees

No one assigned

Labels

type-feature A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@carlbordum @bedevere-bot @ericsnowcurrently @serhiy-storchaka @the-knights-who-say-ni @miss-islington