-
-
Notifications
You must be signed in to change notification settings - Fork 529
Add django.contrib.postgres.forms* #289
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
Add django.contrib.postgres.forms* #289
Conversation
generated via stubgen and cleaned up a bit
mkurnikov
left a comment
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.
Overall, I'd prefer having as little Any throughout the stubs as possible. Better have no method defined than Any. So, please, add types instead.
Thanks!
| from django import forms as forms | ||
|
|
||
| class SimpleArrayField(forms.CharField): | ||
| default_error_messages: Any = ... |
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.
If attribute is defined on the base class, no need to define it on every field.
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.
addressed in 55abc1e
|
Thanks for taking the time to review this. I've cleaned some up methods/attributes that are defined on parent and don't contribute any types. Other methods I derived argument or return types from the code and docs. Please let me know if this looks better, or is there anything else we can improve further? |
|
Thanks, that looks so much better=) About CI errors: those are Liskov violations from Django codebase, you can add method override to the child class with the valid signature (will probably need this is error in Last one is just a line in If any other errors would come up, it's mosly just
PR is good, if you work with forms a lot, and want to contribute more - I would be happy to accept even bigger one, it's been mostly overlooked so far. |
For some reason using the inherited ones fails this test case: https://travis-ci.com/typeddjango/django-stubs/jobs/273220761 I'm not really sure why this is :/
|
Just seen your note after pushing that ^ ... looking again 👀 |
| class DateTimeRangeField(BaseRangeField): ... | ||
| class DateRangeField(BaseRangeField): ... | ||
| class IntegerRangeField(BaseRangeField): | ||
| base_field: Union[Type[forms.Field], forms.Field] = ... |
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.
base_field defined on forms.Field, I think. If it may be both Type[Field] and Field, better fix it there, and remove all this duplication.
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.
addressed in 67d664d
| class JSONString(str): ... | ||
|
|
||
| class JSONField(forms.CharField): | ||
| widget: Union[forms.Widget, Type[forms.Widget]] = ... |
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.
Same as in the comment about base_field
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.
Django is weird ... the field classes expect widget to define a static attribute specifying the widget class, but then instantiate and set the self.widget to the instance in the constuctor 🤦♂ ... so if I pull the above line to forms.Field I start seeing errors like this:
django-sources/tests/postgres_tests/test_ranges.py:723: error: Argument 1 to "render" of "Widget" has incompatible type "str"; expected "Widget"
django-sources/tests/postgres_tests/test_ranges.py:723: error: Too few arguments for "render" of "Widget"
django-sources/tests/postgres_tests/test_ranges.py:727: error: Argument 1 to "render" of "Widget" has incompatible type "str"; expected "Widget"
django-sources/tests/postgres_tests/test_ranges.py:727: error: Argument 2 to "render" of "Widget" has incompatible type "None"; expected "str"
I could only resolve this by pulling defining this in child class:
class BaseRangeField(forms.MultiValueField):
widget: Widget
In that case, I guess it's better to keep the Union on the parent, and add the Argument 1 to "render" of "Widget" has incompatible type "str" error under: enabled_test_modules.py?
67d664d to
4bdec82
Compare
4bdec82 to
0eb73bf
Compare
|
@mkurnikov please feel free to take over this if it still has value. Note: I no longer use Django daily, and don't have the motivation to make this work. |
generated via stubgen and cleaned up a bit