-
Notifications
You must be signed in to change notification settings - Fork 119
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
fix: the other
argument to RelatationDataContent.update(...)
should be optional
#1226
fix: the other
argument to RelatationDataContent.update(...)
should be optional
#1226
Conversation
@tonyandrewmeyer and @benhoyt I appreciate your speedy review as this likely break a few releases queued up for this week. |
The change makes sense, I believe Tony volunteered to tests for this. |
other
argument to RelatationDataContent.update(...)
to be optionalother
argument to RelatationDataContent.update(...)
should be optional
@@ -1722,7 +1712,7 @@ def __getitem__(self, key: str) -> str: | |||
self._validate_read() | |||
return super().__getitem__(key) | |||
|
|||
def update(self, other: _SupportsKeysAndGetItem[str, str], **kwargs: str): | |||
def update(self, other: typing.Any = (), /, **kwargs: str): |
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.
TIL that typeshed doesn't even define dict.update
https://github.com/python/typeshed/blob/main/stdlib/builtins.pyi#L1029
So, all good here!
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.
Super-minor/bikeshedding: I suppose the test could be more concise (either parametrised of performing all 3 updates on the same object/within same test).
Happy to have this fix!
Fair point. I have never really been in the habit of using I considered doing them all in the same test, but felt it was cleaner to have them separate, particularly if only some fail (as is the case before this PR). |
Wow, thanks for the improved solution @dimaqq! |
@tonyandrewmeyer deserves the credit. I've only reviewed the PR 🙇🏻 |
Sorry, it was late and i was blurry-eyed 👀 . Thanks so much @tonyandrewmeyer! |
@IronCore864 Thanks so much also for offering a review |
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.
Fix looks good, thanks @addyess. I agree that the tests have a bit too much duplication, parametrization seems like a good idea here.
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.
Much nicer, thanks
Fixes the signature for
RelationDataContent.update
to matchMutableMapping
, whereother
is optional (a regression introduced in #1883).The type for
other
has been simplified toAny
. It should really beMapping|_SupportsKeysAndGetItem[str,str]
plus a minimal type that supports.values
, but it was already messy pulling in_SupportsKeysAndGetItem
in #1183, and we're just passing this through toMutableMapping
so it doesn't seem like the tight typing is providing enough benefit to justify the complexity of the signature. typeshed has three overloads, so we could match that (as we did in #1883, just incompletely), if that is desirable.Fixes: #1225