-
-
Notifications
You must be signed in to change notification settings - Fork 481
Replace TypeAlias _ClassLevelWidgetT
with descriptor _WidgetTypeOrInstance
#2615
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: master
Are you sure you want to change the base?
Conversation
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.
Looks like a good idea! Thanks!
Please, fix the CI :)
f0c7575
to
097bd62
Compare
_ClassLevelWidgetT
with descriptor WidgetTypeOrInstance
_ClassLevelWidgetT
with descriptor _WidgetTypeOrInstance
…rInstance` for `Field.widget`
097bd62
to
4c4e795
Compare
@sobolevn I think I might need some help there. I checked why the CI is failing and it looks like errors are not even slightly related to what I have changed. I checkouted the current origin/HEAD, ran the test locally and also I got a lot of errors. Is there something obvious I am missing? I believe there might be a problem with the CI itself as I see that on other PR #2616 also fails in the same way. |
Setuptools version 79 got installed, I suppose it probably breaks the multiple |
you can pin older |
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.
Can you please also add several type assertions for this? https://github.com/typeddjango/django-stubs/tree/master/tests/assert_type
@sobolevn I started adding type assertions, and I would like to get you point of view on one thing. I could leave this as typing everything as the most general It provides more precise typing, but I am not sure whether it is not too restrictive. What do you think about that? |
Let's first add the simplest typing possible. We can later update it to cover more cases. |
4426950
to
6c0e4a0
Compare
I have added assert type tests. Seems like this is a bit more complex than I initailly assumed, right now there is a problem with stubtest. Any further guidance would be highly appreciated. |
Based on the amount of failures I would say that this seems overly complex to type properly :( However, I am open to ideas. |
I have made things!
Related issues
None I could find.
This change allows for recognizing when
.widget
is used onField
instance and when on the type itself, allowing for different type annotations in each case.Why even bother?

Example: Currently this correct code does not recognize
is_required
andattrs
onfield.widget
, because it might be a type and not an instance.After the change all types are correctly inferred, and syntax highlighting works as expected:

I hope you find it worth adding.