Skip to content

Conversation

jdufresne
Copy link
Member

No description provided.

Copy link
Member

Choose a reason for hiding this comment

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

You can remove the keys() call as iter(dict.keys()) yields the same results as iter(dict). That is update(base.__dict__).

Copy link
Member

Choose a reason for hiding this comment

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

Just out of interest, dict.keys() is a set-like object providing a view on a dictionary's keys in Python 3. So the cast to set was only needed for Python 2 where dict.keys() returns a list.

Thus the options here are changing to use set.update() and passing the dictionary as-is - as has already been committed, or to just drop the cast to set as it already is set-like which might look a bit less magic. No strong opinion on this however.

Copy link
Member

Choose a reason for hiding this comment

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

Ditto about the keys() call. I'd also try to avoid cloaking the dict builtin while you are around.

set(chain.from_iterable(dict_ for subcontext in self for dict_ in subcontext))

@jdufresne
Copy link
Member Author

jdufresne commented Apr 27, 2017

Thanks for the review. Made suggested changes:

  • Replaced dict.keys() with bare dict
  • Renamed variable dict to d to avoid shadowing builtin

@timgraham timgraham merged commit 7be94e0 into django:master Apr 27, 2017
@jdufresne jdufresne deleted the update branch April 28, 2017 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants