Skip to content

create monkeypatching function for adding get_item dunder #526

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

Merged
merged 16 commits into from Nov 11, 2020
Merged
9 changes: 8 additions & 1 deletion .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,12 @@ repos:
entry: mypy
language: system
types: [ python ]
exclude: "scripts/*"
exclude: "scripts/|django_stubs_ext/"
args: [ "--cache-dir=/dev/null", "--no-incremental" ]
- id: mypy
name: mypy (django_stubs_ext)
entry: mypy
language: system
types: [ python ]
files: "django_stubs_ext/|django_stubs_ext/tests/"
args: [ "--cache-dir=/dev/null", "--no-incremental", "--strict" ]
6 changes: 6 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -104,3 +104,9 @@ The workflow for contributions is fairly simple:
3. make whatever changes you want to contribute.
4. ensure your contribution does not introduce linting issues or breaks the tests by linting and testing the code.
5. make a pull request with an adequate description.

## A Note About Generics

As Django uses a lot of the more dynamic features of Python (i.e. metaobjects), statically typing it requires heavy use of generics. Unfortunately, the syntax for generics is also valid python syntax. For instance, the statement `class SomeClass(SuperType[int])` implicitly translates to `class SomeClass(SuperType.__class_getitem__(int))`. If `SuperType` doesn't define the `__class_getitem__` method, this causes a runtime error, even if the code typechecks.

When adding a new generic class, or changing an existing class to use generics, run a quick test to see if it causes a runtime error. If it does, please add the new generic class to the `_need_generic` list in the [django_stubs_ext monkeypatch function](https://github.com/typeddjango/django-stubs/tree/master/django_stubs_ext/django_stubs_ext/monkeypatch.py)
1 change: 1 addition & 0 deletions dev-requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,5 @@ pre-commit==2.7.1
pytest==6.1.1
pytest-mypy-plugins==1.6.1
psycopg2-binary
-e ./django_stubs_ext
-e .
1 change: 0 additions & 1 deletion django-stubs/contrib/auth/tokens.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,4 @@ class PasswordResetTokenGenerator:
def _num_days(self, dt: date) -> float: ...
def _today(self) -> date: ...


default_token_generator: Any
1 change: 0 additions & 1 deletion django-stubs/contrib/humanize/templatetags/humanize.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,5 @@ class NaturalTimeFormatter:
time_strings: Dict[str, str]
past_substrings: Dict[str, str]
future_substrings: Dict[str, str]

@classmethod
def string_for(cls: Type[NaturalTimeFormatter], value: Any) -> Any: ...
1 change: 0 additions & 1 deletion django-stubs/utils/translation/__init__.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ def ngettext_lazy(singular: str, plural: str, number: Union[int, str, None]) ->
ungettext_lazy = ngettext_lazy

def npgettext_lazy(context: str, singular: str, plural: str, number: Union[int, str, None]) -> str: ...

def activate(language: str) -> None: ...
def deactivate() -> None: ...

Expand Down
49 changes: 49 additions & 0 deletions django_stubs_ext/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# Extensions and monkey-patching for django-stubs

[![Build Status](https://travis-ci.com/typeddjango/django-stubs.svg?branch=master)](https://travis-ci.com/typeddjango/django-stubs)
[![Checked with mypy](http://www.mypy-lang.org/static/mypy_badge.svg)](http://mypy-lang.org/)
[![Gitter](https://badges.gitter.im/mypy-django/Lobby.svg)](https://gitter.im/mypy-django/Lobby)


This package contains extensions and monkey-patching functions for the [django-stubs](https://github.com/typeddjango/django-stubs) package. Certain features of django-stubs (i.e. generic django classes that don't define the `__class_getitem__` method) require runtime monkey-patching, which can't be done with type stubs. These extensions were split into a separate package so library consumers don't need `mypy` as a runtime dependency ([#526](https://github.com/typeddjango/django-stubs/pull/526#pullrequestreview-525798031)).

## Installation

```bash
pip install django-stubs-ext
```

## Usage

In your Django application, use the following code:

```py
import django_stubs_ext

django_stubs_ext.monkeypath()
```

This only needs to be called once, so the call to `monkeypatch` should be placed in your top-level urlconf.

## Version compatibility

Since django-stubs supports multiple Django versions, this package takes care to only monkey-patch the features needed by your django version, and decides which features to patch at runtime. This is completely safe, as (currently) we only add a `__class_getitem__` method that does nothing:

```py
@classmethod
def __class_getitem__(cls, *args, **kwargs):
return cls
```

## To get help

For help with django-stubs, please view the main repository at <https://github.com/typeddjango/django-stubs>

We have a Gitter chat here: <https://gitter.im/mypy-django/Lobby>
If you think you have a more generic typing issue, please refer to <https://github.com/python/mypy> and their Gitter.

## Contributing

The django-stubs-ext package is part of the [django-stubs](https://github.com/typeddjango/django-stubs) monorepo. If you would like to contribute, please view the django-stubs [contribution guide](https://github.com/typeddjango/django-stubs/blob/master/CONTRIBUTING.md).

You can always also reach out in gitter to discuss your contributions!
3 changes: 3 additions & 0 deletions django_stubs_ext/django_stubs_ext/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
from .monkeypatch import monkeypatch

__all__ = ["monkeypatch"]
47 changes: 47 additions & 0 deletions django_stubs_ext/django_stubs_ext/monkeypatch.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
from typing import Any, Generic, List, Optional, Type, TypeVar

import django
from django.contrib.admin import ModelAdmin
from django.contrib.admin.options import BaseModelAdmin
from django.views.generic.edit import FormMixin

_T = TypeVar("_T")


class MPGeneric(Generic[_T]):
"""Create a data class to hold metadata about the gneric classes needing monkeypatching.

The `version` param is optional, and a value of `None` means that the monkeypatch is
version-independent.

This is slightly overkill for our purposes, but useful for future-proofing against any
possible issues we may run into with this method.
"""

version: Optional[int]
cls: Type[_T]

def __init__(self, cls: Type[_T], version: Optional[int] = None):
"""Set the data fields, basic constructor."""
self.version = version
self.cls = cls


# certain django classes need to be generic, but lack the __class_getitem__ dunder needed to
# annotate them: https://github.com/typeddjango/django-stubs/issues/507
# this list stores them so `monkeypatch` can fix them when called
_need_generic: List[MPGeneric[Any]] = [
MPGeneric(ModelAdmin),
MPGeneric(FormMixin),
MPGeneric(BaseModelAdmin),
]


# currently just adds the __class_getitem__ dunder. if more monkeypatching is needed, add it here
def monkeypatch() -> None:
"""Monkey patch django as necessary to work properly with mypy."""
for el in filter(lambda x: django.VERSION[0] == x.version or x.version is None, _need_generic):
el.cls.__class_getitem__ = classmethod(lambda cls, *args, **kwargs: cls)


__all__ = ["monkeypatch"]
14 changes: 14 additions & 0 deletions django_stubs_ext/mypy.ini
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
[mypy]
strict_optional = True
ignore_missing_imports = True
check_untyped_defs = True
warn_no_return = False
show_traceback = True
allow_redefinition = True
incremental = True

plugins =
mypy_django_plugin.main

[mypy.plugins.django-stubs]
django_settings_module = scripts.django_tests_settings
8 changes: 8 additions & 0 deletions django_stubs_ext/pyproject.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
[tool.black]
line-length = 120
include = '\.pyi?$'

[tool.isort]
line_length = 120
multi_line_output = 3
include_trailing_comma = true
8 changes: 8 additions & 0 deletions django_stubs_ext/pytest.ini
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
[pytest]
testpaths =
./tests
addopts =
--tb=native
-s
-v
--cache-clear
17 changes: 17 additions & 0 deletions django_stubs_ext/release.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#!/usr/bin/env bash
set -ex

if [[ -z $(git status -s) ]]
then
if [[ "$VIRTUAL_ENV" != "" ]]
then
pip install --upgrade setuptools wheel twine
python setup.py sdist bdist_wheel
twine upload dist/*
rm -rf dist/ build/
else
echo "this script must be executed inside an active virtual env, aborting"
fi
else
echo "git working tree is not clean, aborting"
fi
7 changes: 7 additions & 0 deletions django_stubs_ext/setup.cfg
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[flake8]
exclude = .*/
select = F401, Y
max_line_length = 120

[metadata]
license_file = ../LICENSE.txt
42 changes: 42 additions & 0 deletions django_stubs_ext/setup.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
from distutils.core import setup

from setuptools import find_packages

with open("README.md") as f:
readme = f.read()

dependencies = [
"django",
]

setup(
name="django-stubs-ext",
version="0.1.0",
description="Monkey-patching and extensions for django-stubs",
long_description=readme,
long_description_content_type="text/markdown",
license="MIT",
url="https://github.com/typeddjango/django-stubs",
author="Simula Proxy",
author_email="[email protected]",
py_modules=[],
python_requires=">=3.6",
install_requires=dependencies,
packages=["django_stubs_ext", *find_packages(exclude=["scripts"])],
classifiers=[
"License :: OSI Approved :: MIT License",
"Operating System :: OS Independent",
"Programming Language :: Python :: 3.6",
"Programming Language :: Python :: 3.7",
"Programming Language :: Python :: 3.8",
"Programming Language :: Python :: 3.9",
"Typing :: Typed",
"Framework :: Django",
"Framework :: Django :: 2.2",
"Framework :: Django :: 3.0",
"Framework :: Django :: 3.1",
],
project_urls={
"Release notes": "https://github.com/typeddjango/django-stubs/releases",
},
)
11 changes: 11 additions & 0 deletions django_stubs_ext/tests/test_monkeypatching.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import django_stubs_ext
from django_stubs_ext.monkeypatch import _need_generic

django_stubs_ext.monkeypatch()


def test_patched_generics() -> None:
"""Test that the generics actually get patched."""
for el in _need_generic:
# This only throws an exception if the monkeypatch failed
assert el.cls[type] == el.cls # `type` is arbitrary
1 change: 1 addition & 0 deletions pytest.ini
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
[pytest]
testpaths =
./tests
./django_stubs_ext/tests
addopts =
--tb=native
-s
Expand Down
1 change: 1 addition & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ def find_stub_files(name: str) -> List[str]:
"mypy>=0.790",
"typing-extensions",
"django",
"django-stubs-ext",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this rather be put as an extra dependency? For example, like so:

setup(
    ...,
    # or another name/way of doing
    extras_require={"full": ["django-stubs-ext"]},
)

Then users could run pip install django-stubs[full] (or the other way around, have full by default and minimal for people that want the minimal, i.e. no monkey patching)

This would prevent adding additional files and dependencies that are not required.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a direct dependency, but you can just ignore it completely. It does not get easier that that.

Copy link
Author

@ghost ghost Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not sure the savings of a relatively small package are worth the possibility of library consumers accidentally missing the [full] part of the install step and wondering what's wrong

i do have a bug report in the mypy repo about this, but it's seen no activity in almost half a year, so i'm not sure we should hold our breath haha

]

setup(
Expand Down