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

create monkeypatching function for adding get_item dunder #526

Merged
merged 16 commits into from Nov 11, 2020
Merged

create monkeypatching function for adding get_item dunder #526

merged 16 commits into from Nov 11, 2020

Conversation

ghost
Copy link

@ghost ghost commented Nov 8, 2020

i have made things!

related issues

closes #507

refs #504, #515

wasn't too sure about the best way to implement this, so i figured i'd make a draft and ask your thoughts

@ghost ghost marked this pull request as ready for review November 8, 2020 01:13
@ghost ghost marked this pull request as draft November 8, 2020 01:24
@ghost
Copy link
Author

ghost commented Nov 8, 2020

if this is good, i'll add in the rest of the stuiff from #504 and #515

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you!

The most interesting question is: how should we distribute this code?
By that I mean:

  1. We can ship it with django-stubs package, but this way we would have to add django-stubs to direct dependencies (not dev-dependencies). And this will also add our dependencies as transitive ones:

    django-stubs/setup.py

    Lines 23 to 27 in aab8acf

    dependencies = [
    "mypy>=0.790",
    "typing-extensions",
    "django",
    ]
    I guess many people might not want mypy to be their direct dependency 🤔

  2. We can create a new package a keep it in sync with django-stubs. It won't have any dependencies at runtime, but this will complicate our tooling and setup. I am not sure if Python has a concept of monorepos.

I would love to hear a feedback on this 👍

@ghost
Copy link
Author

ghost commented Nov 8, 2020

oh look, my favorite part of programming, real world complications!

i think a monorepo is best. if someone makes a class generic, they'd have to submit pull requests to two different repositories, and that just seems way too much for everyone involved. i think a directory structure like this might be best:

.
├── dev-requirements.txt
├── django-source
├── django-stubs
├── django_stubs_ext
│   ├── django_stubs_ext
│   │   └── [code goes here]
│   ├── setup.py
│   └── etc., etc.
├── setup.cfg
├── setup.py
└── etc., etc.

and then when releases happen, just push both to pypi

@sobolevn
Copy link
Member

sobolevn commented Nov 9, 2020

One more thing to consider: #262

At some point in time we plan to support different versions of django, well, differently.
So, this "patcher" thing should be dependent to stubs in different versions.

The good thing is that we are free to patch more classes to be Generic that we actually need.

@ghost
Copy link
Author

ghost commented Nov 9, 2020

i don't think multiple versions would be too difficult with this. make the __need_generic list contain tuples with a version restriction:

__need_generic: List[Tuple[str, Type[Any]]] = [
    ('3', FormMixin),
    ('2', SomeClass),
    ('', VersionIndependentClass),
]

filter(lambda x: str(django.VERSION[0]) == x[0] or x[0] == '', __need_generic)

although i'm sure a more sophisticated solution would also work

@ghost
Copy link
Author

ghost commented Nov 10, 2020

first time writing an actual package, let me know if it looks alright?

i basically copied everything from the root package and deleted what wasn't necessary, then added django_stubs_ext as a requirement to the root package

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

django_stubs_ext/README.md Outdated Show resolved Hide resolved
django_stubs_ext/dev-requirements.txt Outdated Show resolved Hide resolved
django_stubs_ext/django_stubs_ext/monkeypatch.py Outdated Show resolved Hide resolved
django_stubs_ext/django_stubs_ext/monkeypatch.py Outdated Show resolved Hide resolved
django_stubs_ext/requirements.txt Outdated Show resolved Hide resolved
django_stubs_ext/setup.py Outdated Show resolved Hide resolved
@sobolevn sobolevn marked this pull request as ready for review November 10, 2020 09:36
@ghost
Copy link
Author

ghost commented Nov 10, 2020

don't merge just yet, i still need to add the rest of the generic patches :p

.travis.yml Outdated Show resolved Hide resolved
dev-requirements.txt Show resolved Hide resolved
django_stubs_ext/LICENSE.txt Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Nov 11, 2020

alright, i think i'm all set on my end. i can't think of anything else i would want to add or update. does this look good to everyone else?

@ghost
Copy link
Author

ghost commented Nov 11, 2020

writing a release script for it

@ghost ghost changed the title [draft] create monkeypatching function for adding get_item dunder create monkeypatching function for adding get_item dunder Nov 11, 2020
@sobolevn
Copy link
Member

Thanks a lot! Let's merge it! 👍

@sobolevn sobolevn merged commit 0c41d0c into typeddjango:master Nov 11, 2020
@ghost
Copy link
Author

ghost commented Nov 11, 2020

ayyyy fantastic! thanks for the help through all of this :)

@ghost ghost deleted the issue-507 branch December 17, 2020 21:59
@@ -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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Generic Type Error with ModelAdmin when running in strict mode
3 participants