Typing Improvement: Use IntEnum for PhoneNumberFormat, PhoneNumberType, etc.

[Daverball]

Python 3.8.12
phonenumbers 8.12.33

Currently enum like classes like PhoneNumberFormat are using plain classes. For type checking it would be much more convenient if you changed these to Enums or IntEnums (if you need them to pass as int for compatibility reasons). This would allow you to annotate functions like example_number_for_type as example_number_for_type(str, PhoneNumberType) rather than the much more lax example_number_for_type(str, int), which is really not that helpful as a type hint. And even if you don't end up changing the function signatures, it would at least be a win for third party apps, if they could require PhoneNumberType in their functions, rather than a int.

Apart from that the constant values in these classes should really be annotated as Final if you aren't going to change it to an enum. That way type checkers could at the very least do some static analysis knowing that these values won't ever change at runtime. Although currently I don't think it is possible to define a Literal using a Final variable yet. So if I actually wanted to restrict function calls I'd have to define my own IntEnum that just mirrors your classes (or hardcode the Literals using the same integer values, which could break at any point in time).

[AA-Turner]

enum doesn't exist on python 2.5-2.7, I believe -- this library still supports those.

https://www.python.org/dev/peps/pep-0586/#interactions-with-final notes that if a type is specified as Final it should be taken in contexts where Literal type is expected.

I think a reasonable change would be to update the stubs to Literal types with the constants that they represent?

A

[AA-Turner]

https://typing.readthedocs.io/en/latest/source/libraries.html#constants

This seems to suggest that Final[Literal[42]] or whatever is acceptable to type checkers -- would this solve your issues @Daverball?

A

[Daverball]

I can try creating a Union using the Members of PhoneNumberFormat, to see if mypy would accept that as a type.

If it does it would be a reasonable workaround. But it is still a little annoying from a end user's perspective, since I have to manually create that Union and make sure I use that instead of PhoneNumberFormat from phonenumbers.

It would be nice to provide some convenience types within the stubs that contain the necessary Literal Unions, that could be replaced with a type alias once support for Python 2 has been dropped. E.g. PhoneNumberFormatT and PhoneNumberTypeT.

[AA-Turner]

Oh I see -- you want a single name that includes all the valid values. I had thought you were checking on one specific format type, and wanted to ensure that you only got the specific integer literal.

The problem as posed is harder, I don't think there's a good way to solve it. An IntEnum would be the obvious choice, and I would suggest could be adopted when allowed for by the currently supported Pythons (enum was new in 3.3 or 3.4, I think).

A

[Daverball]

I tried constructing an annotation that uses the existing constants, but that does not appear to be possible, at least from my end.

https://pypi.org/project/enum34/ Could be an option.

[Daverball]

@AA-Turner Would you consider pulling in the backported version of Enum linked above for Python 2.5-2.7? At this point it seems better to force an additional dependency on people still using deprecated versions of Python than making the type annotations artificially worse.

[AA-Turner]

I should note I'm not the maintainer -- David is, and he'd be the one to make the decision about this.

My personal opinion is that type hints are and should remain an optional extra -- I wrote them for phonenumbers I found them useful, but significantly changing things that work purely for perhaps better expressability through static typing doesn't seem right. There's also the point that Python versions older than 3.7 are unsupported, and so there's an argument to be made in terms of minimising the change surface there.

As I said above, I think enums might be a good idea, but I wouldn't want to predicate changes solely on type hinting.

A

[Daverball]

@AA-Turner I personally disagree that it would only increase expressability. At least half of the point of static type checking is to find and report invalid arguments reliably. While technically supplying any integer other than one of these faux-Enum constants will not necessarily break your application, you would still prefer to be able to statically validate the arguments rather than to rely on a runtime error or worse yet a default behavior that silently ignores your erroneous argument and leaves you with a difficult to debug corner case.

It would also prevent errors that could not even be caught at runtime currently, such as passing a PhoneNumberFormat instead of a PhoneNumberType and vice versa. Since those share the same integer constants, the functions would currently happily accept one in place of the other, which is certainly a error I could see people making if they don't look too closely.