diff mbox

Refactor the generic_list() view to make it less complicated.

Message ID 1301495296.28156.64.camel@feioso
State Superseded
Headers show

Commit Message

Guilherme Salgado March 30, 2011, 2:28 p.m. UTC
Hi Jeremy,

Thanks for the feedback.  I have some comments below and a patch with
the changes you suggested is attached.

Please let me know if you're happy with it and I'll submit a new version
with all the changes I did.

On Wed, 2011-03-30 at 14:38 +0800, Jeremy Kerr wrote:
> Hi Guilherme,
> 
> You've covered some of the points in a subsequent email, so just responding to 
> the outstanding issues here.
> 
> In general, do you mind if we leave this refactoring until we have the 
> notification branch stable and merged? It'll mean we have to do less to merge 
> the branches together.

Well, I'd think that committing this and building the notification
branch on top of it would be less work than trying to merge them later,
but I haven't seen what's in that branch to tell how badly they'd
conflict, so I guess I'm happy with whatever you prefer.

> 
> > > Are you expecting MultiplePatchForm to be able to handle multiple actions
> > > in future? If not, we should be using MultiplePatchForm.action in the
> > > template too.
> > 
> > I don't foresee that, but I'm happy to change the template to use it.  I
> > guess you mean that to avoid the hard-coding of the action in multiple
> > places?
> 
> Exactly; the action string is defined in one place, and used as-is in multiple 
> templates.

Fair enough; I've renamed .actions to .action and turned it into a
string, which is now used in the template as well.

> 
> > > > +    def save(self, instance):
> > > Why remove the commit parameter? Not that we're using it, but just
> > > curious if there's a reason for this.
> > 
> > I've removed it just because it was unused and untested, but I can
> > revert this if you feel it's worth keeping it.
> > (Most python projects I've worked on seem to take this approach to
> > unused code, given the unnecessary burden of maintaining unused code and
> > how trivial it usually is to add it back if ever needed.)
> 
> The prototype for the save() method is defined in the django.forms.Form API, 
> so I'd rather leave this in.

Ok, I'll revert it, but the signature of the save() method (only defined
in BaseModelForm, AFAICT) is "save(self, commit)", which is different
from ours, and our form does not inherit from BaseModelForm.

> 
> 
> > > > -from patchwork.utils import Order, get_patch_ids, set_patches
> > > > +from patchwork.utils import (
> > > > +    Order, get_patch_ids, bundle_actions, set_bundle)
> > > 
> > > I'd prefer using the backquote for line continuations here
> > 
> > Is that for consistency with the rest of the code?  I ask because
> > python's preferred way (as stated in PEP-8) is to use parentheses.
> 
> Yeah, mostly for consistency.
> 
> BTW, (minor nit) if we're going for maximum PEP-8 compatibility, could you use 
> the remainder of the first line, and indent the following lines to suit? ie:
> 
>     if width == 0 and height == 0 and (color == 'red' or
>                                        emphasis is None):
> 
> rather than:
> 
>     if width == 0 and height == 0 and (
>             color == 'red' or emphasis is None):

Sure, I'll do that.

> 
> 
> > > > +    if request.method == 'POST':
> > > > +        data = request.POST
> > > > +    user = request.user
> > > > +    properties_form = None
> > > > +    if (user.is_authenticated()
> > > > +        and project in user.get_profile().maintainer_projects.all()):
> > > > +        properties_form = MultiplePatchForm(project, data = data)
> > > 
> > > Hm, (I know this is in the original code too), this is duplicating the
> > > Patch.is_editable functionality, and misses some of the cases there.
> > 
> > Do you have any suggestions on how to improve it?  I'd be happy to do
> > it.
> 
> I think it'll be okay to leave as-is for now; I'll review what we need to do 
> in this situation.
> 
> One general thing: could you remove the trailing full-stop from your patch 
> subjects? This means there's one less thing for me to edit when applying.

Yep, I'm happy to do so.  Just out of curiosity, though, is this because
they break something or just
diff mbox

Patch

diff --git a/apps/patchwork/forms.py b/apps/patchwork/forms.py
index 10fffb8..2c57c08 100644
--- a/apps/patchwork/forms.py
+++ b/apps/patchwork/forms.py
@@ -197,7 +197,7 @@  class MultipleBooleanField(forms.ChoiceField):
             raise ValueError('Unknown value: %s' % value)
 
 class MultiplePatchForm(forms.Form):
-    actions = ['update']
+    action = 'update'
     state = OptionalModelChoiceField(queryset = State.objects.all())
     archived = MultipleBooleanField()
 
@@ -206,7 +206,7 @@  class MultiplePatchForm(forms.Form):
         self.fields['delegate'] = OptionalDelegateField(project = project,
                 required = False)
 
-    def save(self, instance):
+    def save(self, instance, commit = True):
         opts = instance.__class__._meta
         if self.errors:
             raise ValueError("The %s could not be changed because the data "
@@ -226,7 +226,8 @@  class MultiplePatchForm(forms.Form):
 
             setattr(instance, f.name, data[f.name])
 
-        instance.save()
+        if commit:
+            instance.save()
         return instance
 
 class UserPersonLinkForm(forms.Form):
diff --git a/apps/patchwork/views/__init__.py b/apps/patchwork/views/__init__.py
index c9b16b9..f66a2bf 100644
--- a/apps/patchwork/views/__init__.py
+++ b/apps/patchwork/views/__init__.py
@@ -60,7 +60,7 @@  def generic_list(request, project, view,
 
         if action in bundle_actions:
             errors = set_bundle(user, project, action, data, ps, context)
-        elif properties_form and action in properties_form.actions:
+        elif properties_form and action == properties_form.action:
             errors = process_multiplepatch_form(
                 properties_form, user, action, ps, context)
         else:
@@ -98,7 +98,7 @@  def generic_list(request, project, view,
 
 def process_multiplepatch_form(form, user, action, patches, context):
     errors = []
-    if not form.is_valid() or action not in form.actions:
+    if not form.is_valid() or action != form.action:
         return ['The submitted form data was invalid']
 
     if len(patches) == 0:
diff --git a/templates/patchwork/patch-list.html b/templates/patchwork/patch-list.html
index 43e0550..12c5d0c 100644
--- a/templates/patchwork/patch-list.html
+++ b/templates/patchwork/patch-list.html
@@ -186,7 +186,7 @@ 
      <tr>
       <td></td>
       <td>
-       <input type="submit" name="action" value="Update"/>
+        <input type="submit" name="action" value="{{patchform.action}}"/>
       </td>
      </tr>
     </table>