Review SupportsInt and SupportsFloat usages in 3rd-party stubs
#11003
Conversation
Co-authored-by: Alex Waygood <[email protected]>
…://github.com/Avasam/typeshed into ConvertibleToInt-ConvertibleToFloat-3rd-party
We still can't go ahead with this just yet, unfortunately. If you look at mypy's vendored stubs in |
Oh my bad, I thought the stubs were updated as well. draft + DO NOT MERGE it is in the mean time. It'll probably accumulate a couple merge conflicts but that's fine they'll be easy to fix. |
This comment has been minimized.
This comment has been minimized.
|
I don't think we should apply |
|
Mypy 1.7 is out now, but I'd still like to wait another week or so before we consider this, to give people time to upgrade to the latest mypy -- being overly hasty with things like this has caused issues in the past. Agree with @Akuli that it might also be good to be opinionated in some places, and continue using |
|
There's a couple places where it's a simple existing union replacement. Splitting it up might make it easier to review intent of other |
that'd be great, yeah :) |
…loat" This reverts commit f2fc06b.
…tibleToInt-ConvertibleToFloat-3rd-party
Avasam
changed the title
ConvertibleToInt and ConvertibleToFloat in 3rd-party stubsSupportsInt and SupportsFloat usages in 3rd-party stubs
This comment has been minimized.
This comment has been minimized.
| @@ -70,10 +74,10 @@ def size() -> Size: ... | |||
|
|
|||
| resolution = size | |||
|
|
|||
| def onScreen(x: _NormalizeableXArg | None, y: SupportsInt | None = None) -> bool: ... | |||
| def onScreen(x: _NormalizeableXArg | None, y: ConvertibleToInt | None = None) -> bool: ... | |||
There was a problem hiding this comment.
Are there any examples in the documentation where y is called with a str argument? Or, is there any particular reason that you know of to pass a str to the y parameter?
It's true that the function at runtime calls int() on the argument passed to y before doing anything with it, so if you pass "10" to the y parameter, it will work. But what's the reason for doing so? SupportsInt isn't a perfect type hint here, but it ensures that the type checker will still emit an error if a user accidentally tries to pass "foo" to the y parameter -- and with this change, it wouldn't.
In general, typeshed tries to put more weight on avoiding false positives than on avoiding false negatives. But in a situation like this, where (as far as I can see) there's no real use case for passing a str, I think we can and should be more opinionated than the function at runtime.
There was a problem hiding this comment.
I don't think so. I'll revert here. How about the normalizable X arg (see _NormalizeableXArg aslias)? It can be a str, but str there means "filename on screen"
There was a problem hiding this comment.
Yeah, I'm fine with that one!
| @@ -40,7 +41,7 @@ class EUI(BaseIdentifier): | |||
| @property | |||
| def value(self) -> int: ... | |||
| @value.setter | |||
| def value(self, value: str | SupportsInt | SupportsIndex) -> None: ... | |||
| def value(self, value: ConvertibleToInt) -> None: ... | |||
There was a problem hiding this comment.
This seems like a good change, since the .value property is explicitly there in order to transform str, bytes objects etc. into int before they're stored on the EUI instance
| @@ -148,14 +148,14 @@ class IPRange(BaseIP, IPListMixin): | |||
| def cidrs(self) -> list[IPNetwork]: ... | |||
|
|
|||
| def iter_unique_ips(*args: IPRange | _IPNetworkAddr) -> Iterator[IPAddress]: ... | |||
| def cidr_abbrev_to_verbose(abbrev_cidr: str | SupportsInt | SupportsIndex) -> str: ... | |||
| def cidr_abbrev_to_verbose(abbrev_cidr: ConvertibleToInt) -> str: ... | |||
There was a problem hiding this comment.
This change seems reasonable to me; the function at runtime is deliberately very permissive in what it accepts
…tibleToInt-ConvertibleToFloat-3rd-party
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
Mypy 1.7 has been released since #10707 .
I have reviewed all usages of
SupportsIntandSupportsFloatthat didn't use the full Union.Direct replacements have been extracted to #11022
Which also makes me realize two things:
Supports*in stdlib could also useConvertibleTo*instead.SupportsIntisn't used anywhere in 3rd-party stubs anymore. ButCONTIBUTING.md's guide section about Protocols providing callable methods explicitly mentions it. Maybe it'd be a good idea to mention theConvertiblealiases also/instead ?