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

[Jeremy Kerr]

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.

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

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


> > > -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):


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

Cheers,


Jeremy