gh-108191: Add support of positional argument in SimpleNamespace constructor by serhiy-storchaka · Pull Request #108195 · python/cpython

serhiy-storchaka

…e constructor

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

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>

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>

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

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)

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.

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

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.

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.

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.

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.