Inherit directly from field & django 1.10 compatibility #19

lucaswiman · 2016-12-29T01:43:55Z

@gregmuellegger This PR significantly simplifies django-superform by having FormSetField and FormField inherit directly from Field, using the builtin Widget.value_from_datadict API. This has a number of advantages:

It's much simpler and less code. This allows SuperForm to inherit directly from Form without need for a custom metaclass.
It uses documented Django APIs, so should continue working more effectively on future versions of Django.
Since composite fields are now just included in form.fields, this fixes There is no way to loop over all superform bound fields #15 and removes the need for Fix iteration over form fields #16 (cc @kmmbvnr).

I also added support for Django 1.10 by updating the test environment and removing a few deprecated kwargs in template rendering. That broke compatibility with Django <1.8. Since all versions before 1.8 have hit end-of-life (and do not even receive security updates), I dropped support for them in the build matrix, along with Python 2.6.

Thanks again for writing this library! It's proven quite useful.

…ith django 1.4.

gregmuellegger · 2016-12-29T21:01:40Z

On the first glance, this is a super sweet change, removing the nastier parts of the implementation! Awesome 🎆

I will try to dive deeper into it soonish. What might be worth adding before merging might be some kind of update-guide, or a description of the public-facing APIs that changed in the CHANGES.rst file.

lucaswiman · 2016-12-29T23:26:13Z

django_superform/widgets.py



 class FormWidget(TemplateWidget):
    template_name = 'superform/formfield.html'
    value_context_name = 'form'

+    @property
+    def media(self):
+        return self.field.form_class().media


Instantiating the form_class / formset_class to get the media can cause some problems. I'll try to see if there's a way around it.

Addressed / partially reverted in 1beb808.

I'd hoped to allow using the widget's `media` property to eventually allow use of FormSetField and FormField on classes that do not inherit from `SuperFormMixin`. Unfortunately, since `media` is a _property_ of the `Form`/`FormSet`, we need an instantiated instance of those. Due to the way field/widget scoping takes place, there's no way to access the "already instantiated" form/formset without hacky stack introspection. Instantiating a formset or form isn't too bad, but can lead to unexpected behavior (like duplicate database queries), so probably best avoided.

…ms and form.formsets.

lucaswiman · 2017-01-04T01:28:53Z

@gregmuellegger After attempting to integrate this into a real-world application, I think this needs some more work before it's ready for review. I'll comment when I'd like you to take another look. Apologies for the delay.

gregmuellegger · 2017-01-09T20:44:12Z

No hurries on that. The internals would be much nicer that way. Just ping me, I'll try to give it a look timely after notify me. Thanks for all the effort you put into this.

lucaswiman · 2017-01-11T23:13:19Z

I've updated the changelog, and I think this is ready for review. For the moment, this PR solves my immediate need of using superform with another forms library that has a conflicting custom metaclass, so it's good enough for now.

To summarize the issues I was considering, using value_from_datadict allows the form/formset to show up in cleaned_data, but makes it hard to ensure that the values in .forms and .formsets are pointing to the same instances. I think this branch is about as far as I can get on that front without removing the forms/formsets attributes, which would break backwards compatibility.

In my opinion, it is worth eventually breaking backwards compatibility (and possibly using other standard django mechanisms like Widget.subwidgets()) in the hopes of upstreaming this functionality to Django core. However, it might make sense to do that in a fork of this library, or as a prototype for inclusion inside Django.

auvipy · 2017-02-24T15:41:01Z

awesome changes. did you tried django 1.11b1 with this ?

lucaswiman · 2017-02-24T22:05:34Z

awesome changes. did you tried django 1.11b1 with this ?

No. That's unrelated to this change and probably better addressed in a different PR.

auvipy · 2017-02-25T04:41:54Z

ok. but will be great to see this pr merged soon

gregmuellegger

Thanks for all your work and patient wait for my response. I think it would be super nice to get rid of the metaclasses.

However I have concerns about returning the actual form instance by value_from_datadict. The problem is that the form itself is not really value that the field operates on. It's more a wrapper element around it.

One case where this surfaces is the use of changed_data property on a form, for example:

>>> from django_superform import *
>>> from django import forms
>>> class OtherForm(forms.Form):
...     field1 = forms.CharField()
...     
... 
>>> class MyForm(SuperForm):
...     field1 = forms.CharField()
...     form1 = FormField(OtherForm)
...     
... 
>>> superform = MyForm()
>>> superform
<MyForm bound=False, valid=Unknown, fields=(field1;form1)>
>>> superform.changed_data
['form1']

So the form indicates that the form1 value changed, even though I haven't bound any initial or new data to the superform instance.

This is due to the way Django implements the changed_data property: https://github.com/django/django/blob/master/django/forms/forms.py#L433
It uses the value_from_datadict.

Maybe there is a solution to make the changed_data property work, but I'm unsure if we would break the expectations of other uses for this function. What's your thought on this? Could we make it somehow behave more as Django expects it to behave? Could this be done by breaking the backwards compatibility you mentioned?

I would be happy to not be compatible with django-superform 0.3 if the implementation would benefit from it. That's why it's not yet 1.0 :)

gregmuellegger · 2017-03-01T06:41:21Z

django_superform/fields.py

-        ``get_form``. You can override this method in subclasses to change
-        the behaviour of the given form class.
-        """
-        return self.form_class


What is the reasoning to remove this method? I liked to have this behaviour separate so you could create a custom subclass for the form if really necessary. Such things have turned out very valueable for other libraries I use (however I'm not sure if I used get_form_class yet in superform to do anything fancy :))

And removing it seems inconsistent with the get_formset_class method of the FormSetField class.

Because I need the form_class in .widget_attrs() below, where the name is not available. It is inconsistent with get_formset_class, though in that case the calling context is controlled by this library, as opposed to Django, so form is available.

gregmuellegger · 2017-03-01T06:42:34Z

django_superform/fields.py

@@ -295,7 +249,7 @@ def get_kwargs(self, form, name):
                kwargs['empty_permitted'] = True
        return kwargs

-    def get_field_name(self, form, name):
+    def get_field_name(self, name):


Why is the form parameter removed here? It's not used in the implementation, but during writing this it made sense to me since this way most methods have the same signature which is easy to remember and contains all information relevant for the field that cannot be accessed via self.

This was some refactoring I did trying to understand the code. I found it difficult to figure out what a lot of parameters were for, which made it harder to understand what information I actually needed for which methods. In particular, get_form_class had a superfluous form parameter, which was inconsistent with the changes I wanted to make, since widget_attrs needed access to the form_class, but didn't have direct access to the form. I may be able to fix that using the "monkeypatching" you mentioned.

It sounds like we have differing ways of thinking about interfaces, but it's your project. 😃 I'll update the code to only remove the superfluous form parameter in cases where it's necessary to remove it.

gregmuellegger · 2017-03-01T06:51:47Z

django_superform/forms.py

@@ -207,26 +153,22 @@ def get_composite_field_value(self, name):
    def _init_composite_field(self, name, field):
        if hasattr(field, 'get_form'):
            form = field.get_form(self, name)
+            field.widget.form = form


Is there a way to avoid the monkeypatching of the form in here? I'm not sure if there is, but I would like to here your thoughts on this.

No, I didn't find a way to do so. The widget is constructed by Django, but we need the form to be accessible from the widget.

gregmuellegger · 2017-03-01T07:18:58Z

Finally got around to review the PR thoroughly. I'm keen to know what you think on the topics brought up.

lucaswiman

Sorry for the delay replying to the code review comments. My son was born shortly after you made them, but I'm working on this PR again this week.

I have concerns about returning the actual form instance by value_from_datadict. The problem is that the form itself is not really value that the field operates on. It's more a wrapper element around it.

To me, it makes sense to me that something called FormField operates on a Form instance. I think this is the sort of thing that value_from_datadict was designed for: a single field corresponding to several HTML form elements. At the level of generality of FormField, there's no "single object" that corresponds to what the form is doing.

Returning the form was an idea I took from django-formsetfield, though you may be right that there's a less hacky solution. One option I'll investigate is returning a dict of data relevant to the form (possibly with the prefix stripped?). I'll report back how that goes.

Maybe there is a solution to make the changed_data property work, but I'm unsure if we would break the expectations of other uses for this function. What's your thought on this? Could we make it somehow behave more as Django expects it to behave? Could this be done by breaking the backwards compatibility you mentioned?

This is a good catch! I think this is a general problem with value_from_datadict, which is presumably why Django provided a solution in Field.has_changed. In the Django docs for value_from_datadict, their example is a date widget with multiple "subwidgets" on the year, month and day. DateField understand the output of a date, and can tell when it has changed, but I think I need to implement that behavior myself in FormField.has_changed regardless of what value_from_datadict returns. I'll add a test case reproducing your example of `has_changed as well.

lucaswiman · 2018-02-25T20:44:47Z

django_superform/fields.py

-        ``get_form``. You can override this method in subclasses to change
-        the behaviour of the given form class.
-        """
-        return self.form_class


Because I need the form_class in .widget_attrs() below, where the name is not available. It is inconsistent with get_formset_class, though in that case the calling context is controlled by this library, as opposed to Django, so form is available.

lucaswiman · 2018-02-25T20:51:05Z

django_superform/fields.py

@@ -295,7 +249,7 @@ def get_kwargs(self, form, name):
                kwargs['empty_permitted'] = True
        return kwargs

-    def get_field_name(self, form, name):
+    def get_field_name(self, name):


This was some refactoring I did trying to understand the code. I found it difficult to figure out what a lot of parameters were for, which made it harder to understand what information I actually needed for which methods. In particular, get_form_class had a superfluous form parameter, which was inconsistent with the changes I wanted to make, since widget_attrs needed access to the form_class, but didn't have direct access to the form. I may be able to fix that using the "monkeypatching" you mentioned.

It sounds like we have differing ways of thinking about interfaces, but it's your project. 😃 I'll update the code to only remove the superfluous form parameter in cases where it's necessary to remove it.

lucaswiman · 2018-02-25T20:54:44Z

django_superform/forms.py

@@ -207,26 +153,22 @@ def get_composite_field_value(self, name):
    def _init_composite_field(self, name, field):
        if hasattr(field, 'get_form'):
            form = field.get_form(self, name)
+            field.widget.form = form


No, I didn't find a way to do so. The widget is constructed by Django, but we need the form to be accessible from the widget.

Lucas Wiman added 11 commits December 28, 2016 16:04

Increase verbosity of py.test to make it more user friendly on failures.

e0e00a1

Update to inherit directly from field.

d8580b9

Eliminate need for SuperForm.composite_fields.

8ccae82

Remove base_composite_fields and need for custom metaclasses.

7c9beaf

Remove support for old end-of-lifed versions due to incompatibility w…

30deb03

…ith django 1.4.

Add Django 1.10 to the build matrix.

c124126

Upgrade django test dependencies.

aef4da9

Add new required TEMPLATES setup.

2171536

Remove usage of deprecated kwargs

e8dacbe

update travis environments.

052fd69

Fix flake8 failures.

81caaa9

lucaswiman commented Dec 29, 2016

View reviewed changes

Lucas Wiman added 5 commits December 29, 2016 15:48

Override add_prefix.

1cedcc1

Remove need for get_form_class.

4663c07

Remove unused form parameter from get_field_name.

2dbd0f2

Ensure that the values returned by the widget is the same as form.for…

6d7814e

…ms and form.formsets.

Update changelog.

5b0f9df

gregmuellegger reviewed Mar 1, 2017

View reviewed changes

ArtemBernatskyy mentioned this pull request May 26, 2017

__init__() got an unexpected keyword argument 'instance' #22

Open

lucaswiman commented Feb 25, 2018

View reviewed changes

Update test matrix to include 1.11 and 2.0, as well as python 3.6.

fedaf89

lucaswiman added 3 commits March 1, 2018 15:57

Handle extra renderer argument.

53f729a

Update for Django 1.11 compatibility.

9766411

Updates for django 2.0.

c118e9d

lucaswiman closed this Nov 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inherit directly from field & django 1.10 compatibility #19

Inherit directly from field & django 1.10 compatibility #19

lucaswiman commented Dec 29, 2016 •

edited

Loading

gregmuellegger commented Dec 29, 2016

lucaswiman Dec 29, 2016

lucaswiman Dec 29, 2016 •

edited

Loading

lucaswiman commented Jan 4, 2017

gregmuellegger commented Jan 9, 2017

lucaswiman commented Jan 11, 2017

auvipy commented Feb 24, 2017

lucaswiman commented Feb 24, 2017

auvipy commented Feb 25, 2017

gregmuellegger left a comment •

edited

Loading

gregmuellegger Mar 1, 2017

lucaswiman Feb 25, 2018

gregmuellegger Mar 1, 2017

lucaswiman Feb 25, 2018

gregmuellegger Mar 1, 2017

lucaswiman Feb 25, 2018

gregmuellegger commented Mar 1, 2017

lucaswiman left a comment

lucaswiman Feb 25, 2018

lucaswiman Feb 25, 2018

lucaswiman Feb 25, 2018

Inherit directly from field & django 1.10 compatibility #19

Inherit directly from field & django 1.10 compatibility #19

Conversation

lucaswiman commented Dec 29, 2016 • edited Loading

gregmuellegger commented Dec 29, 2016

Choose a reason for hiding this comment

lucaswiman Dec 29, 2016 • edited Loading

Choose a reason for hiding this comment

lucaswiman commented Jan 4, 2017

gregmuellegger commented Jan 9, 2017

lucaswiman commented Jan 11, 2017

auvipy commented Feb 24, 2017

lucaswiman commented Feb 24, 2017

auvipy commented Feb 25, 2017

gregmuellegger left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregmuellegger commented Mar 1, 2017

lucaswiman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucaswiman commented Dec 29, 2016 •

edited

Loading

lucaswiman Dec 29, 2016 •

edited

Loading

gregmuellegger left a comment •

edited

Loading