Skip to content

Conversation

@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Aug 21, 2023

SimpleNamespace({'a': 1, 'b': 2}) and SimpleNamespace([('a', 1), ('b', 2)]) are now the same as SimpleNamespace(a=1, b=2).


📚 Documentation preview 📚: https://cpython-previews--108195.org.readthedocs.build/

…e constructor SimpleNamespace({'a': 1, 'b': 2}) and SimpleNamespace([('a', 1), ('b', 2)]) are now the same as SimpleNamespace(a=1, b=2).
@serhiy-storchaka serhiy-storchaka force-pushed the SimpleNamespace-pos-arg branch from 213c74d to c761180 Compare August 21, 2023 07:44
Copy link
Member

@carljm carljm left a comment

Choose a reason for hiding this comment

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

This looks like a convenient option to provide in the constructor, with no downside, and established precedent in the dict constructor.

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

Thanks! The "polish" commit was a nice clean-up; it made the code more readable, IMO.

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
@ericsnowcurrently
Copy link
Member

I have no real objections. I had considered doing this originally but figured it could wait. Here we are. 😄

@rhettinger rhettinger removed their request for review September 3, 2023 22:54
@rhettinger rhettinger self-assigned this Sep 7, 2023
@serhiy-storchaka
Copy link
Member Author

I tried to implement SimpleNamespace.__replace__, and the code may be simpler with this feature:

args = (self.__dict__,) return type(self)(*args, **kwargs)

instead of

d = {} d.update(self.__dict__) d.update(kwargs) return type(self)(**d)
@serhiy-storchaka
Copy link
Member Author

On other hand, I decided to use other way (#109175), which is more consistent with pickling and copying:

result = type(self)() result.__dict__.update(self.__dict__) result.__dict__.update(kwargs) return result

So this is no longer a blocker.

@bedevere-app
Copy link

bedevere-app bot commented Jan 18, 2024

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

serhiy-storchaka and others added 6 commits March 7, 2024 12:48
Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com> Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
@serhiy-storchaka
Copy link
Member Author

I have made the requested changes; please review again.

@bedevere-app
Copy link

bedevere-app bot commented Mar 7, 2024

Thanks for making the requested changes!

@erlend-aasland, @ericsnowcurrently, @AA-Turner, @carljm: please review the changes made to this pull request.

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

I left one comment about something mostly unrelated to this PR, so I don't consider it a blocker.

@serhiy-storchaka
Copy link
Member Author

It would be good to get @rhettinger's feedback, but this PR already received many positive feedbacks, and I already invested in this issue so much, that I'm going to merge it anyway, even if initially I was not so interested in this.

Copy link
Member

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Seems to be a reasonable amount of interest in this, and plenty of review approvals. Can we go ahead and merge for 3.13?

@carljm
Copy link
Member

carljm commented Apr 24, 2024

Looks like a merge conflict needs to be resolved.

@serhiy-storchaka serhiy-storchaka merged commit 93b7ed7 into python:main Apr 24, 2024
@serhiy-storchaka serhiy-storchaka deleted the SimpleNamespace-pos-arg branch April 24, 2024 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

10 participants