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. |
tonyandrewmeyer
changed the title
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.
TIL that typeshed doesn't even define dict.update
https://github.com/python/typeshed/blob/main/stdlib/builtins.pyi#L1029
So, all good here!
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 |
Fixes the signature for
RelationDataContent.updateto matchMutableMapping, whereotheris optional (a regression introduced in #1883).The type for
otherhas been simplified toAny. It should really beMapping|_SupportsKeysAndGetItem[str,str]plus a minimal type that supports.values, but it was already messy pulling in_SupportsKeysAndGetItemin #1183, and we're just passing this through toMutableMappingso 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