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

feat: adding typehints #683

Merged
merged 1 commit into from
Mar 10, 2023
Merged

Conversation

abczzz13
Copy link
Contributor

@abczzz13 abczzz13 commented Mar 8, 2023

What

Added typehints, as mentioned in #664.

Discussion

  • Used AuthUser as a TypeVar to type the dynamic django user model. Went with the assumption that the User model would always be inheriting from the AbstractBaseUser, but not sure if that is really always the case. Also added the TokenUser to it to account for the JWTStatelessUserAuthentication. Not sure if this is the best implementation for this.
  • Mixin and inheritance issues. Currently, the problem with mixins and some inherintances, is that the other (parent) class can’t always be deduced by mypy, which gives some warnings.
  • Usage of type: ignore at a few places. Not sure if that’s something we would want, as it could also mask problems that might want to be tackled in the future.
  • Not really happy with the readability of using Union and Optional, the “|” union operator could make it a bit better, but was only added in 3.10

Issues related to the tox configuration

  • Noticed django2.2 references in the envlist of the tox.ini file, which caused some errors running the tests locally. As django2.2 is not supported anymore now, this could maybe be updated?
  • Additionally, the djmain tests were also failing locally due to this:
* The undocumented ``django.http.multipartparser.parse_header()`` function is
 removed. Use ``django.utils.http.parse_header_parameters()`` instead.

Not sure if that’s something we should worry about. It seems that djmain doesn’t play nice with drf3.13, which has been fixed in drf3.14.

Could possibly be picked up in a separate PR. Let me know what you think of it.

Tests

Did not create any additional tests, as I didn't expect it to be relevant for adding typehints. Made sure all existing tests still ran as expected (except for the above mentioned issues). Also tried to minimize the number of warnings from mypy, but was not able to fix all of them (would also depend on the mypy configuration).

Future…

  • Possibly adding Type checking with mypy in the future? Currently, mypy is still returning some warnings. So this would require some additional work regarding typing. Additionally, a mypy configuration has to be defined. Not sure if this is something you would like to see in the future.

Finally

Feedback/suggestions are welcome! Will happily make adjustments wherever necessary. :)

@abczzz13 abczzz13 marked this pull request as ready for review March 8, 2023 12:42
@abczzz13
Copy link
Contributor Author

abczzz13 commented Mar 8, 2023

Regarding the tox configuration, I just noticed there were already talks about removing djmain and adding support for the drf3.14 in PR #623.

@Andrew-Chen-Wang
Copy link
Member

Our API doesn't change that much, so adding mypy would be appreciated if time permits. Greatly appreciate this effort!

@Andrew-Chen-Wang
Copy link
Member

Thanks again! I'm going to merge this for now, and then it would be great to have a separate mypy PR

@Andrew-Chen-Wang Andrew-Chen-Wang merged commit 8258b5f into jazzband:master Mar 10, 2023
@abczzz13
Copy link
Contributor Author

Sure thing! Will get started on adding mypy as pre-commit hook.

I'm not sure however, how far we would want to go to make sure the mypy check also passes. I guess it also depends a bit on the configuration we use for mypy. But I can imagine we would still get some warnings out of it, which can either be ignored with type: ignore, or possibly need some code changes. But I'm not sure if that should be part of the scope.

Either way, I will start on it and see into which issues I'll be running.

Thanks for approving the PR!

@PedroPerpetua
Copy link
Contributor

Hi! I'd like to run this locally and take a look myself, as skimming trough I noticed some things that I think could be better typed; but I'm having trouble setting up a development environment to be able to run mypy trough.

Do you have any guide / instructions on how you did it?

@PedroPerpetua
Copy link
Contributor

PedroPerpetua commented Mar 14, 2023

@PedroPerpetua https://django-rest-framework-simplejwt.readthedocs.io/en/latest/development_and_contributing.html

Unfortunately mypy fails because it's missing django-stubs, which are not included or configured in the test package (I imagine they'd be getting in with the pre-commit hook @abczzz13 will work on).

I'll try to setup a dummy project with everything setup and install the djangorestframework-simplejwt from there and run mypy against it. But if @abczzz13 has a better / simpler method, I'd be all ears.

Edit: also, while at it, this should be developed against any specific Django / DRF / python versions?

@abczzz13
Copy link
Contributor Author

Hey Pedro! I can fully imagine it can be better typed, especially when taking into account myp with django and drf stubs. Any help on this would be welcome :)

I made a beginning with adding mypy precommit hook, including the stubs, which indeed caused a whole bunch typing warnings which needed some working.

Let me know if you want to see/use anything that I have, or that you will start with a blank slate.

In the tox.ini file you can see all the specific versions it gets tested with.

@PedroPerpetua
Copy link
Contributor

Hey Pedro! I can fully imagine it can be better typed, especially when taking into account myp with django and drf stubs. Any help on this would be welcome :)

I made a beginning with adding mypy precommit hook, including the stubs, which indeed caused a whole bunch typing warnings which needed some working.

Let me know if you want to see/use anything that I have, or that you will start with a blank slate.

In the tox.ini file you can see all the specific versions it gets tested with.

I'm happy to setup a clean "test" django repository, alongside my own fork of this repo where I do my own improvements on it. When it's ready, I'll link it here. I'll start from the latest master commits.

@PedroPerpetua
Copy link
Contributor

Here is what I'll be using:
Clean Django project with mypy configured to develop typehints against: https://github.com/PedroPerpetua/simplejwt-clean-django
My own fork of the project: https://github.com/PedroPerpetua/djangorestframework-simplejwt

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.

4 participants