Skip to content
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

Add a typeshed alias to the type accepted by int constructor #10707

Merged
merged 4 commits into from
Sep 23, 2023

Conversation

hamdanal
Copy link
Contributor

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@JelleZijlstra
Copy link
Member

Not sure we ever came up with a general stance on this, but it's risky to immediately use this in third-party libraries, because we'll release those immediately, while the _typeshed changes will only go out to users with the next release of their type checker (likely at least a month away for mypy, for example). Therefore, it may be better to only add the alias to _typeshed now, but not use it in third-party libraries until some time after the first mypy release that includes the new alias.

@AlexWaygood
Copy link
Member

@JelleZijlstra, it looks to me like this PR is safe from that perspective, since it only adds TODO comments to the third-party stubs; it doesn't make any substantive changes except to the stdlib

@JelleZijlstra
Copy link
Member

Sorry for that! Should have actually looked at the changes instead of just the list of changed files.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

This is a pretty complicated alias that seems pretty commonly used in typeshed, so I'm persuaded that it makes sense to have a common definition in _typeshed.

I'm not wildly keen on the name. It's descriptive, but, idk... feels slightly clunky. But, not sure I can think of anything better. (ConvertibleToInt? Intable?)

@Daverball
Copy link
Contributor

Daverball commented Sep 13, 2023

I dislike that AcceptedByInt kind of gives the impression that calling int with a value of this type can't fail, but for str it definitely still can. But ConvertibleToInt is not really better in that regard and MayConvertToInt is clunky in its own right and Integerable/Intable are just barely real words that are easy to get tripped up by. ConvertibleToInt would probably be my preference if it's between any of those though.

Also why not add an alias for float as well, while we're at it? The type union it accepts is similarly complex and we wouldn't risk being disruptive to users stuck on older versions of type checkers with newer version of dependent stubs twice by adding these two aliases staggered, even if float is maybe slightly more rare than int. types-openpyxl has an alias for both already too.

@hamdanal
Copy link
Contributor Author

I'm not wildly keen on the name. It's descriptive, but, idk... feels slightly clunky. But, not sure I can think of anything better. (ConvertibleToInt? Intable?)

I think both AcceptedByInt and ConvertibleToInt covey the meaning of the type well, I don't have a preference.

In general I like the idea of the alias and I don't care much about the name. If you, the maintainers, see this alias useful, I am happy to go with whatever name you choose

I dislike that AcceptedByInt kind of gives the impression that calling int with a value of this type can't fail, but for str it definitely still can.

Types are about preventing TypeErrors not ValueErrors (for most cases). Also this is just an alias, the real type already says it accepts any string. I am not sure this discussion is relevant here.

Also why not add an alias for float as well, while we're at it?

Good question, I happen to have stumbled on the int case but not the float one. If the maintainers support this idea I can add it as well (or feel free to add in a separate PR)

Some stats (on the current main branch):

int aliases (5 definitions + int.__new__):

File Alias name Nb of uses
stdlib/multiprocessing/util.pyi no alias, just inline comment 1
stubs/openpyxl/openpyxl/descriptors/base.pyi _ConvertibleToInt 674
stubs/psycopg2/psycopg2/_psycopg.pyi _AcceptedByInt 3
stubs/pyasn1/pyasn1/type/univ.pyi _SizedIntegerable 3
stubs/python-xlib/Xlib/protocol/rq.pyi _IntNew 1

In typeshed, _typeshed.SupportsTrunc is only ever used to define this type

float aliases (3 definitions + float.__new__):

File Alias name Nb of uses
stubs/libsass/sass.pyi _ConvertsToFloat 5
stubs/openpyxl/openpyxl/descriptors/base.pyi _ConvertibleToFloat 165
stubs/python-xlib/Xlib/ext/xinput.pyi _Floatable 3

@Daverball
Copy link
Contributor

Daverball commented Sep 16, 2023

Types are about preventing TypeErrors not ValueErrors (for most cases). Also this is just an alias, the real type already says it accepts any string. I am not sure this discussion is relevant here.

I don't disagree with you or have an issue with the signature containing str, although I quite like the ability to restrict string literals to a given RegEx pattern, which is possible in some type systems, but that is besides the point.

The original signature did not have a name for the type union, so it does not convey any expectations about the nature of this type, while AcceptedByInt does convey an expectation, which could be misinterpreted. After all a ValueError is just a more narrow rejection of the input, compared to a TypeError, which rejects an entire type, rather than individual values of a type (although not everyone uses these errors consistently, ValueError will often be used regardless of whether it could be a TypeError)

You could imagine a type union of this name containing only the types that can't fail when passed into int and then have a broader type union alongside it for the types that could cause a ValueError.

When you're given an error in a type checker you only get the name of the type alias, so it should be as unambiguous as possible, in order to be helpful, otherwise writing out the individual types in the union is preferrable, the name should be descriptive enough that you shouldn't have to look up it's definition.

But it is a very minor nitpick and unlikely to actually trip up anyone in a harmful way, since what is accepted by int is generally well understood. I just wanted to explain why I personally dislike the current name.

Good question, I happen to have stumbled on the int case but not the float one. If the maintainers support this idea I can add it as well (or feel free to add in a separate PR)

I have a feeling either type alias could be used a lot more often, than they currently are, even within the stdlib, it's just that most of the time when someone sees int(value) in an implementation they settle for float | str to cover the most common use-case, even if there's technically a broader type that's still valid. If there isn't already a type alias in typeshed, I doubt many people will go out of their way to define one, so providing one should be a positive change, regardless of how heavily they're currently used.

@Avasam
Copy link
Collaborator

Avasam commented Sep 20, 2023

I'm partial to "ConvertibleToInt" given I added the alias to openpyxl, but won't care too much either way. I support the addition of the alias and is something I've raised in the past as well due to Python's type system inability to reference other methods' parameters' type. (for example in typescript you can use that to ensure wrapper methods don't fall out of sync)

I think you missed adding a comment to the one in https://github.com/python/typeshed/blob/main/stubs/psycopg2/psycopg2/_psycopg.pyi#L11 though

Should an equivalent for float be made as well? I can see 4 stubs using a "convertible to float" alias (libsass, openpyxl, python-xlib and pyscreeze). And made me realize the pyscreeze one is missing ReadableBuffer from the alias.


To @JelleZijlstra 's point, to do the actual change we'll have to wait at least until the next mypy release. And then there's no way to enforce it for users who don't pin their type dependencies (see #6843 (comment) and typeshed-internal/stub_uploader#96 , at least it's documented in the distribution's description). See #5835 for a possible proper solution

@hamdanal
Copy link
Contributor Author

hamdanal commented Sep 20, 2023

ConvertibleToInt it is. I also removed all the TODO notes that I added earlier, when we want to use the alias, we can refer to this PR to find its uses or simply grep for them. I also added ConvertibleToFloat, I think the arguments made for it are convincing and I don't see why int should be treated differently.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@JelleZijlstra JelleZijlstra merged commit 2b323be into python:main Sep 23, 2023
58 checks passed
@hamdanal hamdanal deleted the int-new-alias branch September 23, 2023 06:45
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.

5 participants