Add type hints, fix mypy issues (#198) #228

jwhitlock · 2024-06-24T21:53:52Z

This add mypy to the test matrix, and type hints to all the code, fixing #198. mypy required some code changes, and I've rebased them out into separate commits, which could be added before the big one that hints all the functions.

I've built on top of PR #227, which fixes configuration issues with tox.ini. In that PR, I disabled pypy testing against Django 3.2, which was consistently breaking. I thought these changes fixed that, but they still fail 1 out of 10 times locally, so I'm keeping them disabled.

To run mypy locally:

pip install -e .[tests,typing]

* Specify how to install Django 3.2. These tests are currently running against later Django versions. * Add basepython entries for pypy. This fixes running tox locally. * Fix the cpython version mapping in [gh-actions]. The github action tests for cpython versions are running against the latest Django, instead of the set of possible Django versions.

There are a few test failures with pypy and Django 3.2. Since 3.2 is out of long-term support, drop these from the test matrix.

mypy complains when reading or setting a attribute that is not defined on the class, such as HttpRequest.csp_nonce. This updates the code to use getattr and setattr to access these dynamically added attributes and for Django settings.

Both startswith() and parser.parse_statements take a tuple rather than a list.

Althought the code `template.render(context)` looked similar, mypy complained that Django's Template could not take a dict. Rather than switch on types, refactor `make_context` and `make_template` into `render`, which hides the typing details between Django templates and extension templates like Jinja2.

robhudson

This is awesome, thanks @jwhitlock!

asottile-sentry

the py.typed file isn't included in the packaging I think? so these types won't be available to users of the package

asottile-sentry · 2024-07-02T15:47:32Z

csp/constants.py

@@ -15,12 +17,12 @@
 class Nonce:
    _instance = None

-    def __new__(cls, *args, **kwargs):
+    def __new__(cls: Type["Nonce"], *args: Any, **kwargs: Any) -> "Nonce":


the way to write this is:

def __new__(cls, *args: Any, **kwargs: Any) -> Self:

(Self is importable from typing_extensions for the pythons supported here)

Yes, this seems to work. Do you want to submit the PR?

jwhitlock

Thanks @asottile-sentry. Do you want to submit a PR?

the py.typed file isn't included in the packaging I think? so these types won't be available to users of the package

I was worried about this too, but I think no further config is needed. When I follow the deploy steps in the GitHub action:

pip install build
python -m build -v .

I see the py.typed file getting added to the package data:

...
* Building sdist...
...
copying csp/models.py -> django_csp-4.0b1/csp
copying csp/py.typed -> django_csp-4.0b1/csp
copying csp/utils.py -> django_csp-4.0b1/csp
...
* Building wheel...
...
copying build/lib/csp/py.typed -> build/bdist.macosx-14.4-arm64/wheel/csp
...
adding 'csp/py.typed'
...

jwhitlock · 2024-07-02T20:01:23Z

csp/constants.py

@@ -15,12 +17,12 @@
 class Nonce:
    _instance = None

-    def __new__(cls, *args, **kwargs):
+    def __new__(cls: Type["Nonce"], *args: Any, **kwargs: Any) -> "Nonce":


Yes, this seems to work. Do you want to submit the PR?

asottile-sentry · 2024-07-02T20:05:46Z

Thanks @asottile-sentry. Do you want to submit a PR?

the py.typed file isn't included in the packaging I think? so these types won't be available to users of the package

I was worried about this too, but I think no further config is needed. When I follow the deploy steps in the GitHub action:
pip install build
python -m build -v .
I see the py.typed file getting added to the package data:
...
* Building sdist...
...
copying csp/models.py -> django_csp-4.0b1/csp
copying csp/py.typed -> django_csp-4.0b1/csp
copying csp/utils.py -> django_csp-4.0b1/csp
...
* Building wheel...
...
copying build/lib/csp/py.typed -> build/bdist.macosx-14.4-arm64/wheel/csp
...
adding 'csp/py.typed'
...

the automatic inclusion of py.typed needs at least setuptools>=69: https://github.com/pypa/setuptools/blob/e9f0be98ea4faaba4a7b2d07ba994a81fde8f42f/NEWS.rst#v6900

jwhitlock · 2024-07-02T21:02:21Z

the automatic inclusion of py.typed needs at least setuptools>=69: https://github.com/pypa/setuptools/blob/e9f0be98ea4faaba4a7b2d07ba994a81fde8f42f/NEWS.rst#v6900

I think we're good then, build installs setuptools>=61.2 so should pick up the latest

https://github.com/mozilla/django-csp/actions/runs/9589504359/job/26443444411#step:6:14

asottile-sentry · 2024-07-03T02:47:24Z

it relies on an experimental feature and it breaks in environments which don't have setuptools 69+ available -- such as building from source

either the >= should be adjusted (limits where it can be built from source) or it should use the proper package_data stanza to support older setuptools

robhudson force-pushed the add-types-198 branch from 9b80d67 to 7153092 Compare June 28, 2024 21:06

jwhitlock force-pushed the add-types-198 branch from 7153092 to 9b80d67 Compare June 28, 2024 21:19

jwhitlock added 12 commits June 28, 2024 14:21

Remove pypy from Django 3.2 testing

f4f83ad

There are a few test failures with pypy and Django 3.2. Since 3.2 is out of long-term support, drop these from the test matrix.

Add mypy for type checking

441fdf9

Handle case where config is None

3acd7b7

Use getattr, setattr for dynamic attribute access

5854441

mypy complains when reading or setting a attribute that is not defined on the class, such as HttpRequest.csp_nonce. This updates the code to use getattr and setattr to access these dynamically added attributes and for Django settings.

Use tuples where requested

abf8c1b

Both startswith() and parser.parse_statements take a tuple rather than a list.

Add type hints

035844a

Fix Sphinx doc generation without setuptools

26c03b1

Add pip install -e ".[dev]"

42125a5

Update docs for typing, etc.

f247464

Add PEP 561 py.typed file

0b728aa

robhudson force-pushed the add-types-198 branch from 9b80d67 to 0b728aa Compare June 28, 2024 21:21

robhudson added 2 commits June 28, 2024 14:29

Bump Django dependency to 4.2+

c2a4317

Replace HttpResponse type with HttpResponseBase

0afd172

robhudson force-pushed the add-types-198 branch from 794c819 to 0afd172 Compare June 28, 2024 22:16

Update CHANGES file

a14b51c

robhudson force-pushed the add-types-198 branch from c828d9e to a14b51c Compare July 1, 2024 17:01

robhudson approved these changes Jul 1, 2024

View reviewed changes

jwhitlock mentioned this pull request Jul 1, 2024

Update tox.ini for Django 3.2, pypy, github actions #227

Closed

robhudson merged commit 2bb3e6d into mozilla:main Jul 1, 2024
9 checks passed

robhudson mentioned this pull request Jul 2, 2024

interested in adding typing (mypy) support? #198

Closed

asottile-sentry reviewed Jul 2, 2024

View reviewed changes

jwhitlock commented Jul 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type hints, fix mypy issues (#198) #228

Add type hints, fix mypy issues (#198) #228

jwhitlock commented Jun 24, 2024

robhudson left a comment

asottile-sentry left a comment

asottile-sentry Jul 2, 2024

jwhitlock Jul 2, 2024

jwhitlock left a comment •

edited

Loading

jwhitlock Jul 2, 2024

asottile-sentry commented Jul 2, 2024

jwhitlock commented Jul 2, 2024

asottile-sentry commented Jul 3, 2024

Add type hints, fix mypy issues (#198) #228

Add type hints, fix mypy issues (#198) #228

Conversation

jwhitlock commented Jun 24, 2024

robhudson left a comment

Choose a reason for hiding this comment

asottile-sentry left a comment

Choose a reason for hiding this comment

asottile-sentry Jul 2, 2024

Choose a reason for hiding this comment

jwhitlock Jul 2, 2024

Choose a reason for hiding this comment

jwhitlock left a comment • edited Loading

Choose a reason for hiding this comment

jwhitlock Jul 2, 2024

Choose a reason for hiding this comment

asottile-sentry commented Jul 2, 2024

jwhitlock commented Jul 2, 2024

asottile-sentry commented Jul 3, 2024

jwhitlock left a comment •

edited

Loading