Skip to content

Have UserDict.__init__() implicitly check for updating w/ bool(kwargs) instead of len() #12139

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
miss-islington merged 1 commit into from
Apr 2, 2019
Merged

Have UserDict.__init__() implicitly check for updating w/ bool(kwargs) instead of len() #12139

miss-islington merged 1 commit into from
Apr 2, 2019

Conversation

@0bsearch
Copy link
Contributor

@0bsearch 0bsearch commented Mar 2, 2019 •
edited by brettcannon
Loading

Semantically the same, but more idiomatic by checking against kwargs instead of len(kwargs).

@0bsearch 0bsearch requested a review from rhettinger as a code owner March 2, 2019 15:01
eamanu
eamanu reviewed Mar 3, 2019
View reviewed changes
Copy link
Contributor

@eamanu eamanu left a comment

Choose a reason for hiding this comment

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

look good. But why do you use this check and not use the len?

Are there some bpo for this?

@0bsearch
Copy link
Contributor Author

0bsearch commented Mar 3, 2019

I don't think there's BPO for this, just found this while researching ABC-related things.

Reason for this change – I think checks vs len is not idiomatic for python (this is point to discuss, for sure).

@remilapeyre
Copy link

remilapeyre commented Mar 5, 2019

I don't think the test is needed anyway, if kwargs is empty self.update({}) will do nothing.

@0bsearch
Copy link
Contributor Author

0bsearch commented Mar 5, 2019

@remilapeyre that was my original intention, but .update machinery is way too heavy, removing test gives heavy performance degradation for small dicts. And I guess, ~90% of usecases builds empty dicts and than populate.

@remilapeyre
Copy link

remilapeyre commented Mar 5, 2019

Thanks for clarifying!

remilapeyre
remilapeyre approved these changes Mar 5, 2019
View reviewed changes
Copy link

@remilapeyre remilapeyre left a comment

Choose a reason for hiding this comment

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

The change is obviously correct and I don't think it needs a b.p.o submission nor a NEWS entry.

I'm not sure it's needed either but a core developer will either merge or close the PR during the next review.

@brettcannon brettcannon changed the title UserDict constructor: check against kwargs instead of len Have UserDict.__init__() implicitly check for updating w/ bool(kwargs) instead of len() Apr 2, 2019
@miss-islington miss-islington merged commit 76b387b into python:master Apr 2, 2019
@brettcannon
Copy link
Member

brettcannon commented Apr 2, 2019

Thanks!

@0bsearch 0bsearch deleted the idiomatic_userdict_update branch April 2, 2019 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brettcannon brettcannon brettcannon approved these changes

@rhettinger rhettinger Awaiting requested review from rhettinger

+2 more reviewers

@eamanu eamanu eamanu left review comments

@remilapeyre remilapeyre remilapeyre approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

skip issue skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@0bsearch @remilapeyre @brettcannon @eamanu @the-knights-who-say-ni @bedevere-bot @miss-islington