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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Increased test coverage of django.utils.datastructures to 100%. #13494
Conversation
bf56a02
to
e3c183c
Compare
40c3896
to
ca4024f
Compare
Thanks for this @pope1ni. Looks good at first glance. Let me just give it a ponder. (Will respond fully for next week.) |
9326f91
to
246a04a
Compare
246a04a
to
551c671
Compare
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.
Hey @pope1ni. I think this is great. 👍
The 4 main commits make sense separately I think.
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.
@pope1ni Thanks 👍 just some nitpicks
551c671
to
9d452fb
Compare
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.
@pope1ni Thanks👍
"Removed..." in the third commit😉
Auuugh! 🙀 😂 |
Co-authored-by: Nick Pope <nick.pope@flightdataservices.com>
Co-authored-by: Mads Jensen <mje@inducks.org>
If the warning provided was an instance of Exception, then it would be used as-is. In practice this is untested, unused and ImmutableList is an undocumented internal datastructure.
Changes in behavior include: - Accepting iteration over empty sequences, updating nothing. - Accepting iterable of 2-tuples providing key-value pairs. - Failing with the same or comparable exceptions for invalid input. Notably this replaces the previous attempt to catch TypeError which was unreachable as the call to .items() resulted in AttributeError on non-dict objects.
9d452fb
to
966b5b4
Compare
I'm really being mocked now: Test failure in |
Supersedes #12442. I split out the tests for
MultiValueDict
andOrderedSet
into two commits and added additional missing test coverage to these. Also in relation toMultiValueDict.update()
, in that PR, Mads said:I came to the conclusion that it wasn't possible to trigger
TypeError
as the call to.items()
was always going to triggerAttributeError
for every other common type. Further, I noted that the behaviour ofMultiValueDict.update()
was inconsistent with and more strict thandict.update()
. To my understanding, the main reason for overriding the.update()
method is to a) ensure that the values are always lists, and b) special case to handle provision of anotherMultiValueDict
instance.So I added a commit that makes the behaviour as consistent with
dict.update()
as possible. A few exceptions are slightly differently worded due to the waydict.update()
is implemented in C versus this Python implementation forMultiValueDict.update()
, but they are comparable..I've also attached a quick script I used to compare output of
dict.update()
andMultiValueDict.update()
as we'll as before and after output:Script to compare behaviour.
Comparison before change in behavior.
Comparison after change in behavior.
Finally, there is an extra commit that removes support for providing an instance of
Exception
to thewarning
argument forImmutableList
. This is untested, undocumented and unused in Django itself. It didn't seem worth adding a test and preserving this behavior.