Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Closed
asfaltboy wants to merge 4 commits into from
Closed

Add django.contrib.postgres.forms* #289

asfaltboy wants to merge 4 commits into from

Conversation

@asfaltboy
Copy link
Contributor

@asfaltboy asfaltboy commented Jan 1, 2020

generated via stubgen and cleaned up a bit

@asfaltboy
Add django.contrib.postgres.forms
38f6957 
generated via stubgen and cleaned up a bit
mkurnikov
mkurnikov requested changes Jan 1, 2020
View reviewed changes
Copy link
Member

@mkurnikov mkurnikov left a 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!

django-stubs/contrib/postgres/forms/array.pyi Outdated
from django import forms as forms

class SimpleArrayField(forms.CharField):
default_error_messages: Any = ...
Copy link
Member

@mkurnikov mkurnikov Jan 1, 2020

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.

Copy link
Contributor Author

@asfaltboy asfaltboy Jan 7, 2020

Choose a reason for hiding this comment

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

addressed in 55abc1e

@asfaltboy
Copy link
Contributor Author

asfaltboy commented Jan 7, 2020

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?

@mkurnikov
Copy link
Member

mkurnikov commented Jan 7, 2020

Thanks, that looks so much better=)

About CI errors:

django-sources/tests/postgres_tests/test_array.py:887: error: Too few arguments for "get_context" of "Widget"
django-sources/tests/postgres_tests/test_array.py:921: error: Too few arguments for "get_context" of "Widget"

those are Liskov violations from Django codebase, you can add method override to the child class with the valid signature (will probably need # type: ignore in stubs), or just add ignore line to the proper place in enabled_test_modules.py

django-sources/tests/postgres_tests/test_json.py:536: error: Incompatible types in assignment (expression has type "Type[Input]", base class "Field" defined the type as "Widget")

this is error in widget: attribute somewhere in the forms.Field I think? Could you fix it in this PR?

Last one is just a line in enabled_test_modules, one of those is definitely an invalid assignment

        class SplitDateTimeRangeField(pg_forms.DateTimeRangeField):
            base_field = forms.SplitDateTimeField

        class SplitForm(forms.Form):
            field = SplitDateTimeRangeField()

If any other errors would come up, it's mosly just

  1. If it's error in tests, or wrong type annotation under assertRaises, add lines with message to ignores.
  2. If it's inexpressible, or requires some complicated Union, especially in the return type - just set to Any.

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.

@asfaltboy
Respecify some inherited attribute types
de05701 
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 :/
@asfaltboy
Copy link
Contributor Author

asfaltboy commented Jan 7, 2020

Just seen your note after pushing that ^ ... looking again 👀

mkurnikov
mkurnikov reviewed Jan 7, 2020
View reviewed changes
django-stubs/contrib/postgres/forms/ranges.pyi Outdated
class DateTimeRangeField(BaseRangeField): ...
class DateRangeField(BaseRangeField): ...
class IntegerRangeField(BaseRangeField):
base_field: Union[Type[forms.Field], forms.Field] = ...
Copy link
Member

@mkurnikov mkurnikov Jan 7, 2020

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.

Copy link
Contributor Author

@asfaltboy asfaltboy Jan 7, 2020

Choose a reason for hiding this comment

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

addressed in 67d664d

mkurnikov
mkurnikov reviewed Jan 7, 2020
View reviewed changes
django-stubs/contrib/postgres/forms/jsonb.pyi Outdated
class JSONString(str): ...

class JSONField(forms.CharField):
widget: Union[forms.Widget, Type[forms.Widget]] = ...
Copy link
Member

@mkurnikov mkurnikov Jan 7, 2020 •
edited
Loading

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

Copy link
Contributor Author

@asfaltboy asfaltboy Jan 7, 2020 •
edited
Loading

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?

@asfaltboy asfaltboy force-pushed the add-django.contrib.postgres.forms branch from 67d664d to 4bdec82 Compare January 7, 2020 20:42
@asfaltboy asfaltboy force-pushed the add-django.contrib.postgres.forms branch from 4bdec82 to 0eb73bf Compare January 7, 2020 21:15
@asfaltboy
Copy link
Contributor Author

asfaltboy commented Nov 17, 2020

@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.

@asfaltboy asfaltboy closed this Nov 17, 2020
kalekseev pushed a commit to kalekseev/django-stubs that referenced this pull request Mar 25, 2025
@collinanderson
admin formfield_for_dbfield returns FormField, not ModelField (typedd…
d3fdb42 
…jango#289)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mkurnikov mkurnikov mkurnikov approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@asfaltboy @mkurnikov