stefanfoulis / django-phonenumber-field

A django model and form field for normalised phone numbers using python-phonenumbers
MIT License
1.47k stars 317 forks source link

Support typing #562

Open last-partizan opened 1 year ago

last-partizan commented 1 year ago

Hi, are you interested in adding typing support to your project?

Right now with latest pyright and django-types field in a model behaves like str, becouse it's based on CharField.

To fix this, i'm using .pyi with following content locally:

_T = TypeVar("_T", bound=PhoneNumber | None)

class PhoneNumberField(models.CharField[_T]):  # type: ignore
    attr_class = PhoneNumber
    descriptor_class = PhoneNumberDescriptor
    default_validators = ...
    description = ...
    @overload
    def __new__(
        cls, *args, null: Literal[False] = ..., **kwargs
    ) -> PhoneNumberField[PhoneNumber]: ...
    @overload
    def __new__(
        cls, *args, null: Literal[True] = ..., **kwargs
    ) -> PhoneNumberField[PhoneNumber | None]: ...
    @overload
    def __new__(cls, *args, **kwargs) -> PhoneNumberField[PhoneNumber]: ...
    def __new__(cls, *args, **kwargs) -> PhoneNumberField[PhoneNumber | None]: ...

I can make a PR with a changes if you're interested.

francoisfreitag commented 1 year ago

We should add mypy to the CI to verify types, before to officially support type hints. If you’re up for it, that would be greatly appreciated!

last-partizan commented 1 year ago

Sure, but first, some questions.

  1. what exactly do you want to check in CI? You want to type-check all source code, or you want to type check some test code to ensure that added types are working? I used second approach when adding types to pytest-decopatch: https://github.com/smarie/python-decopatch/pull/27/files
  2. Do you want mypy specifically, or pyright is fine too? I'm more used to pyright, and it has nice --outputjson option, so we can test against snapshot as in my linked PR.

Currently, pyright has better type-checking capabilities, here's example run on this package.

``` > src/django-phonenumber-field 562-mypy-ci > mypy phonenumber_field phonenumber_field/widgets.py:21: error: Incompatible types in assignment (expression has type "None", variable has type Module) [assignment] Found 1 error in 1 file (checked 8 source files) > src/django-phonenumber-field 562-mypy-ci > pyright phonenumber_field /home/serg/src/django-phonenumber-field/phonenumber_field/formfields.py /home/serg/src/django-phonenumber-field/phonenumber_field/formfields.py:30:38 - error: No parameter named "region" (reportGeneralTypeIssues) /home/serg/src/django-phonenumber-field/phonenumber_field/formfields.py:30:26 - error: Object of type "Widget" is not callable (reportGeneralTypeIssues) /home/serg/src/django-phonenumber-field/phonenumber_field/formfields.py:32:26 - error: Object of type "Widget" is not callable (reportGeneralTypeIssues) /home/serg/src/django-phonenumber-field/phonenumber_field/formfields.py:35:21 - error: Cannot assign member "input_type" for type "type[Widget]"   Member "input_type" is unknown (reportGeneralTypeIssues) /home/serg/src/django-phonenumber-field/phonenumber_field/formfields.py:35:21 - error: Cannot assign member "input_type" for type "Widget"   Member "input_type" is unknown (reportGeneralTypeIssues) /home/serg/src/django-phonenumber-field/phonenumber_field/formfields.py:40:52 - error: Cannot access member "as_national" for type "PhoneNumber"   Member "as_national" is unknown (reportGeneralTypeIssues) /home/serg/src/django-phonenumber-field/phonenumber_field/formfields.py:40:52 - error: "as_national" is not a known member of "None" (reportOptionalMemberAccess) /home/serg/src/django-phonenumber-field/phonenumber_field/phonenumber.py /home/serg/src/django-phonenumber-field/phonenumber_field/phonenumber.py:110:51 - error: Cannot access member "is_valid" for type "str"   Member "is_valid" is unknown (reportGeneralTypeIssues) /home/serg/src/django-phonenumber-field/phonenumber_field/phonenumber.py:110:27 - error: Cannot access member "format_as" for type "str"   Member "format_as" is unknown (reportGeneralTypeIssues) /home/serg/src/django-phonenumber-field/phonenumber_field/phonenumber.py:110:73 - error: Cannot access member "raw_input" for type "str"   Member "raw_input" is unknown (reportGeneralTypeIssues) /home/serg/src/django-phonenumber-field/phonenumber_field/serializerfields.py /home/serg/src/django-phonenumber-field/phonenumber_field/serializerfields.py:28:46 - error: Cannot access member "is_valid" for type "str"   Member "is_valid" is unknown (reportGeneralTypeIssues) /home/serg/src/django-phonenumber-field/phonenumber_field/widgets.py /home/serg/src/django-phonenumber-field/phonenumber_field/widgets.py:19:12 - warning: Import "babel" could not be resolved from source (reportMissingModuleSource) /home/serg/src/django-phonenumber-field/phonenumber_field/widgets.py:43:28 - error: Argument of type "tuple[str, str]" cannot be assigned to parameter "__object" of type "tuple[Literal[''], Literal['---------']]" in function "append"   "str" cannot be assigned to type "Literal['']"   "str" cannot be assigned to type "Literal['---------']" (reportGeneralTypeIssues) /home/serg/src/django-phonenumber-field/phonenumber_field/widgets.py:121:16 - error: Cannot assign member "_region" for type "PhoneNumber"   Member "_region" is unknown (reportGeneralTypeIssues) /home/serg/src/django-phonenumber-field/phonenumber_field/widgets.py:121:16 - error: Cannot assign member "_region" for type "Literal['']"   Member "_region" is unknown (reportGeneralTypeIssues) /home/serg/src/django-phonenumber-field/phonenumber_field/widgets.py:155:62 - error: Argument of type "int | None" cannot be assigned to parameter "country_code" of type "int" in function "region_codes_for_country_code"   Type "int | None" cannot be assigned to type "int"     Type "None" cannot be assigned to type "int" (reportGeneralTypeIssues) /home/serg/src/django-phonenumber-field/phonenumber_field/widgets.py:174:62 - error: Argument of type "int | None" cannot be assigned to parameter "country_code" of type "int" in function "region_codes_for_country_code"   Type "int | None" cannot be assigned to type "int"     Type "None" cannot be assigned to type "int" (reportGeneralTypeIssues) 16 errors, 1 warning, 0 informations ```

But, if we're planning to only verify that after adding types - we're getting what we expecting - both are good choices.

last-partizan commented 1 year ago

https://github.com/stefanfoulis/django-phonenumber-field/pull/563

Here's basic mypy integration, and it already passes the tests with only two minor changes.

francoisfreitag commented 1 year ago

That’s great, thanks! :star:

  1. Ideally, I would rather type check all the source code, so that type hints help directly when reading the code. That might be a second step, I’m guessing it requires more work.
  2. Most errors reported by pyright look like false positives, not issues of interest. I would rather stick with mypy, since that’s what Django might be using in the future according to https://github.com/django/deps/pull/65/, and the target of django-stubs.
last-partizan commented 1 year ago

Now the hard part!

I'll try to add types for fields, my previous idea was working with pyright but i'll need to do some changes for mypy :)

What's preferred way of adding types, inline or in separate .pyi files?

francoisfreitag commented 1 year ago

I vote for inline, they provide information when reading the source that way, without having to jump back and forth between the .pyi or relying on IDE support.

HasanAshab commented 4 months ago

Any update? PR not merged yet.