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

[Guilherme Salgado]

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 

-- 
Guilherme Salgado <https://launchpad.net/~salgado>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: refactor.diff
Type: text/x-patch
Size: 2599 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/patchwork/attachments/20110330/ad7ecf4a/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/patchwork/attachments/20110330/ad7ecf4a/attachment.pgp>