From patchwork Wed Mar 30 14:28:16 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Guilherme Salgado X-Patchwork-Id: 88926 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from ozlabs.org (localhost [127.0.0.1]) by ozlabs.org (Postfix) with ESMTP id 7E9D0B6F44 for ; Thu, 31 Mar 2011 01:28:25 +1100 (EST) Received: from adelie.canonical.com (adelie.canonical.com [91.189.90.139]) by ozlabs.org (Postfix) with ESMTP id 1FC0FB6F10 for ; Thu, 31 Mar 2011 01:28:22 +1100 (EST) Received: from youngberry.canonical.com ([91.189.89.112]) by adelie.canonical.com with esmtp (Exim 4.71 #1 (Debian)) id 1Q4wNh-0004Vj-7h; Wed, 30 Mar 2011 14:28:21 +0000 Received: from [187.126.171.108] (helo=feioso) by youngberry.canonical.com with esmtpsa (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1Q4wNg-0007j7-JD; Wed, 30 Mar 2011 14:28:21 +0000 Received: by feioso (Postfix, from userid 1001) id 43E1D422ED; Wed, 30 Mar 2011 11:28:17 -0300 (BRT) Subject: Re: [PATCH] Refactor the generic_list() view to make it less complicated. From: Guilherme Salgado To: Jeremy Kerr In-Reply-To: <201103301438.22732.jk@ozlabs.org> References: <20110225193135.9524.64622.stgit@localhost6.localdomain6> <201103091100.39807.jk@ozlabs.org> <1299671552.9049.52.camel@feioso> <201103301438.22732.jk@ozlabs.org> Date: Wed, 30 Mar 2011 11:28:16 -0300 Message-ID: <1301495296.28156.64.camel@feioso> Mime-Version: 1.0 X-Mailer: Evolution 2.30.3 Cc: patchwork@lists.ozlabs.org, patches@linaro.org X-BeenThere: patchwork@lists.ozlabs.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Patchwork development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org Sender: patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org 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 --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 @@ - +