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

Support typing #562

Open
last-partizan opened this issue Jul 21, 2023 · 7 comments
Open

Support typing #562

last-partizan opened this issue Jul 21, 2023 · 7 comments

Comments

@last-partizan
Copy link
Contributor

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
Copy link
Collaborator

francoisfreitag commented Jul 21, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

#563

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

@francoisfreitag
Copy link
Collaborator

That’s great, thanks! ⭐

  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 First draft of DEP for static-ish typechecking for Django django/deps#65, and the target of django-stubs.

@last-partizan
Copy link
Contributor Author

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
Copy link
Collaborator

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.

last-partizan added a commit to last-partizan/django-phonenumber-field that referenced this issue Aug 22, 2023
@HasanAshab
Copy link

Any update? PR not merged yet.

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

No branches or pull requests

3 participants