Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-108191: Add support of positional argument in SimpleNamespace constructor#108195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
gh-108191: Add support of positional argument in SimpleNamespace constructor#108195
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…e constructorSimpleNamespace({'a': 1, 'b': 2}) and SimpleNamespace([('a', 1), ('b', 2)])are now the same as SimpleNamespace(a=1, b=2).
213c74d
toc761180
CompareThere was a problem hiding this 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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this 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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
I have no real objections. I had considered doing this originally but figured it could wait. Here we are. 😄 |
I tried to implement args= (self.__dict__,)returntype(self)(*args,**kwargs) instead of d= {}d.update(self.__dict__)d.update(kwargs)returntype(self)(**d) |
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)returnresult So this is no longer a blocker. |
When you're done making the requested changes, leave the comment: |
Uh oh!
There was an error while loading.Please reload this page.
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>
I have made the requested changes; please review again. |
Thanks for making the requested changes! @erlend-aasland,@ericsnowcurrently,@AA-Turner,@carljm: please review the changes made to this pull request. |
There was a problem hiding this 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.
Uh oh!
There was an error while loading.Please reload this page.
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. |
There was a problem hiding this 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?
Looks like a merge conflict needs to be resolved. |
93b7ed7
intopython:mainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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/