Skip to content

make BaseModelAdmin generic to properly type methods dealing with models #504

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 3 commits into from Oct 27, 2020
Merged

Conversation

ghost
Copy link

@ghost ghost commented Oct 27, 2020

i have made things!

related issues

closes #482

this is my first time really working with mypy like this, apologies if i made any mistakes that i didn't catch

@ghost
Copy link
Author

ghost commented Oct 27, 2020

black failed in ci, but when i ran it locally it wanted to make a lot of completely unrelated changes, which i figured wouldn't really be good for this pr

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! 👍

Please, reformat this file only, I think that it is ok to trust black to do its job.
Maybe it has a new version / rule to apply.

@ghost
Copy link
Author

ghost commented Oct 27, 2020

i added a new typevar _ModelT that's bound to Model, and if an argument to a method looked like it should be generic, i typed it as _ModelT. i wasn't super sure on some of them, but i figured it would be better to be safe, since i don't think it'll really create any issues

@ghost ghost changed the title make BaseModelAdmin generic to properly type the obj argument of ModelAdmin.delete_model make BaseModelAdmin generic to properly type methods dealing with models Oct 27, 2020
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 work! 👍

Let's add a single test case for ModelAdmin class! Currently, we are terrible at test-coverage 😞
You can take some inspiration from https://github.com/typeddjango/django-stubs/blob/master/test-data/typecheck/contrib/admin/test_options.yml

@ghost
Copy link
Author

ghost commented Oct 27, 2020

i'd love to add a test case, but i'm not quite clear on what i would test. just write subclass of ModelAdmin to make sure it typechecks?

@sobolevn
Copy link
Member

@ghost
Copy link
Author

ghost commented Oct 27, 2020

would something like this work? reveal_type worked, but the output looked really unintuitive, so i figured it might be best to just have the field typecheck

if that's good with you, i'll add something that doesn't typecheck to make sure the constraint works properly

@sobolevn sobolevn merged commit ffb6551 into typeddjango:master Oct 27, 2020
@sobolevn
Copy link
Member

Thanks a lot!

@sobolevn
Copy link
Member

if that's good with you, i'll add something that doesn't typecheck to make sure the constraint works properly

Feel free to send a new PR with an extra test 👍

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.

BaseModelAdmin should be a generic class
1 participant