-
Notifications
You must be signed in to change notification settings - Fork 45
Add type hints using basedpyright #85
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
base: main
Are you sure you want to change the base?
Conversation
inertia/tests/testapp/urls.py
Outdated
@@ -14,7 +14,7 @@ | |||
path("defer-group/", views.defer_group_test), | |||
path("merge/", views.merge_test), | |||
path("complex-props/", views.complex_props_test), | |||
path("share/", views.share_test), | |||
path("share/", views.share_test), # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using type: ignore
it is best practice to specify the code(s) to ignore.
inertia/utils.py
Outdated
@@ -13,23 +13,22 @@ def model_to_dict(model): | |||
|
|||
|
|||
class InertiaJsonEncoder(DjangoJSONEncoder): | |||
def default(self, value): | |||
if hasattr(value.__class__, "InertiaMeta"): | |||
def default(self, o): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably a better name given that's what the parent object uses, but this isn't really related to typing. Why not type this function since it's being modified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, pyright was complaining exactly about the name being different from the base class. I think it's a good idea to add typing there as well, I'll change it
pyproject.toml
Outdated
@@ -8,7 +8,7 @@ version = "1.2.0" | |||
description = "Django adapter for the InertiaJS framework" | |||
authors = [{name="Brandon Shar", email="[email protected]"}] | |||
license = "MIT" | |||
requires-python = ">=3.9" | |||
requires-python = ">=3.10" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm always for using more modern versions, but this is an important change that perhaps should be done elsewhere?
If it is because of typing features, why it would be best to just use from __future__ import annotations
to enable lazy evaluations of type hints. Then as long as the type checker is run with a modern version of Python it will be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good call. django-stubs requires Python 3.10, so I could not add the dependency bumping the Python version. As Python 3.9 is reaching EOL on October, I thought it was harmless to do that, but I agree it should be done as another PR. I'll open it later today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose that is coming up very soon. I'm all for dropping 3.9, it just feels like something that should be dropped in a bigger release. Awesome, thanks!
pyproject.toml
Outdated
@@ -51,3 +53,6 @@ unfixable = ["B", "SIM"] | |||
[tool.pytest.ini_options] | |||
DJANGO_SETTINGS_MODULE = "inertia.tests.settings" | |||
django_find_project = false | |||
|
|||
[tool.basedpyright] | |||
typeCheckingMode = "standard" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm quite partial to mypy instead of pyright. I think its more the standard, but anything that encourages typing I'm here for.
I think it would be important to add a way to run the type checking too. Perhaps even in the CI. It could be added later too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that is a good point. I used mypy before, but it was somehow very slow on a project I was working on and pyright was much faster. For a library, I agree that using the most established solution is probably best.
I'll change to mypy and update the CI as well. Probably it's better to follow how it's being done there as a way to run the type checking.
inertia/http.py
Outdated
@@ -143,7 +149,7 @@ def build_first_load(self, data): | |||
"inertia_layout": layout, | |||
**context, | |||
}, | |||
self.request, | |||
self.request.request, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a functional change. Was the code incorrect before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't say it is incorrect, it works, but I'm not sure it can satisfy the type checker. render_to_string
expects a HttpRequest
from Django, and self.request
is a InertiaRequest, which wraps the request it but is not a subclass from HttpRequest
, so I got a type error.
Because this is a Django function, it does not seem to me that it is accessing anything Inertia related, so I forwarded the inner request. Since the tests passed, this seemed correct to me, and the type checking error was gone. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, good point. I only raised the question as it could impact functionality. Perhaps this will no longer be needed if #84 is merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just going to tag the same, thanks @sopelj . I'll see if we can get that PR merged asap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just merged #84 so that should help us clean this up a bit!
@@ -1,6 +1,7 @@ | |||
from functools import wraps | |||
from http import HTTPStatus | |||
from json import dumps as json_encode | |||
from typing import Any, Callable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lazy annotations and if TYPE_CHECKING
might be a nice way to avoid unessecary imports.
inertia/http.py
Outdated
def inertia(component): | ||
def decorator(func): | ||
def inertia(component: str): | ||
def decorator(func: Callable[..., HttpResponse | InertiaResponse | dict[str, Any]]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For decorators it, would be ideal to use ParamSpec
and TypeVar to ensure the types are correct in the project where it is used. But I suppose this could be added in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't know about ParamSpec
, I'll see if I can use that instead, it does seem useful in this case. Thanks again for the review!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's very handy for decorators as it allows you to use the args and kwargs without knowing what they are. I can always add them in a future PR too once the setup for typing has been added too. My pleasure thanks for you work.
Love to see some type checking added! I almost opened a PR to add it as well. If I can assist in anyway I'd be happy to. I am partial to mypy, though and am unfamiliar with pyright. Adding a way to run the type checking would be great too. I typically use mypy with pre-commit and github actions. |
Thanks @mrgalopes and @sopelj for the review! @mrgalopes any thoughts on the review comments? I'm not familiar with pyright, I've only used mypy before (that doesn't mean we have to use it here though) |
Thanks for the review, @sopelj! Sorry for the delay, I was traveling and could not work on this. I'll address each one. @BrandonShar from what I used before, mypy tended to be much slower than pyright, but it is the most used. There is also ty from the makers of ruff and uv, but it's not stable yet. I'll switch to mypy and see if there is any difference, and also update the CI as per @sopelj 's suggestion |
@mrgalopes No worries! Some are just suggestions too. |
FWIW, I've been switching projects to |
0cd619e
to
dd65e9f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds comprehensive type hints to the Inertia Django package for better IDE support and type checking. The changes introduce mypy as a type checker with strict mode enabled and django-stubs for Django-specific type support.
- Adds type annotations to all functions, methods, and class attributes across the codebase
- Configures mypy with strict type checking in pyproject.toml
- Refactors some code patterns for better type safety (e.g., replacing validate_type utility with inline type checks)
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
pyproject.toml | Adds mypy, django-stubs, and types-requests dependencies with mypy configuration |
.github/workflows/ci.yml | Adds mypy type checking to CI pipeline |
inertia/utils.py | Adds type hints to utility functions and JSON encoder |
inertia/share.py | Adds type hints to shared prop functionality |
inertia/settings.py | Adds type hints to settings class |
inertia/prop_classes.py | Adds type hints to property classes |
inertia/middleware.py | Adds type hints to middleware class and improves message handling logic |
inertia/http.py | Adds comprehensive type hints and refactors type validation |
inertia/helpers.py | Adds type hints and removes validate_type utility function |
inertia/tests/testapp/urls.py | Adds type ignore comment for view function |
Comments suppressed due to low confidence (2)
pyproject.toml:41
- mypy version 1.17.0 does not exist. The latest stable version as of my knowledge cutoff is 1.13.0. Please use a valid version like "^1.13.0".
mypy = "^1.17.0"
pyproject.toml:43
- The version 2.32.4.20250611 appears to include a future date (20250611 suggests June 11, 2025). This version likely does not exist. Please use a valid version that is currently available.
types-requests = "^2.32.4.20250611"
) | ||
if args: | ||
super().__init__( | ||
*args, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conditional logic for handling args creates inconsistent behavior. When args is provided, the content parameter is not passed to the parent constructor, which could result in empty response content. The content should be passed in both branches.
*args, | |
*args, | |
content=content, |
Copilot uses AI. Check for mistakes.
Hello! Thanks for integrating Inertia with Django, I found it very helpful.
This adds type hints for better IDE support, and uses basedpyright for type checking as a dev dependency. I used the "standard" type checking mode, which is not as strict.
Let me know what you think! Thanks!