Message ID | BLU437-SMTP2635B3D20FF3AC576033EBA3F40@phx.gbl |
---|---|
State | Accepted |
Headers | show |
Hi Stephen, I've had a quick look at the patches labelled trivial. For those patches en bloc: Reviewed-by: Daniel Axtens <dja@axtens.net> Regards, Daniel Stephen Finucane <stephenfinucane@hotmail.com> writes: > The are two somewhat significant changes: > > * The behavior of 'Bundle.add_patch' is changed. Previously this would > raise an exception if the provided patch already existed in the > bundle. Since this code was only used in one location, change this to > return the BundlePatch if valid else None and change the calling code > to check return value instead of catching the exception. > * Use a context manager to open the config file in pwclient. This loses > a little granularity in error messaging, but this is a worthy > compromise. > > Signed-off-by: Stephen Finucane <stephenfinucane@hotmail.com> > --- > patchwork/bin/pwclient | 11 ++--------- > patchwork/models.py | 4 +++- > patchwork/notifications.py | 3 ++- > patchwork/paginator.py | 4 ++-- > patchwork/templatetags/listurl.py | 6 +++--- > patchwork/views/__init__.py | 6 +++--- > patchwork/views/bundle.py | 15 ++++----------- > patchwork/views/mail.py | 4 ++-- > patchwork/views/patch.py | 14 +++++++------- > patchwork/views/user.py | 4 ++-- > patchwork/views/xmlrpc.py | 6 +++--- > 11 files changed, 33 insertions(+), 44 deletions(-) > > diff --git a/patchwork/bin/pwclient b/patchwork/bin/pwclient > index 791c511..b63db53 100755 > --- a/patchwork/bin/pwclient > +++ b/patchwork/bin/pwclient > @@ -294,17 +294,10 @@ def action_get(rpc, patch_id): > fname = "%s.%d" % (base_fname, i) > i += 1 > > - try: > - f = open(fname, "w") > - except: > - sys.stderr.write("Unable to open %s for writing\n" % fname) > - sys.exit(1) > - > - try: > + with open(fname, 'w') as f: > f.write(unicode(s).encode("utf-8")) > - f.close() > print('Saved patch to %s' % fname) > - except: > + except IOError: > sys.stderr.write("Failed to write to %s\n" % fname) > sys.exit(1) > > diff --git a/patchwork/models.py b/patchwork/models.py > index b1c6d8d..4c0aa88 100644 > --- a/patchwork/models.py > +++ b/patchwork/models.py > @@ -580,12 +580,14 @@ class Bundle(models.Model): > # see if the patch is already in this bundle > if BundlePatch.objects.filter(bundle=self, > patch=patch).count(): > - raise Exception('patch is already in bundle') > + return > > bp = BundlePatch.objects.create(bundle=self, patch=patch, > order=max_order + 1) > bp.save() > > + return bp > + > def public_url(self): > if not self.public: > return None > diff --git a/patchwork/notifications.py b/patchwork/notifications.py > index 5420401..16152ef 100644 > --- a/patchwork/notifications.py > +++ b/patchwork/notifications.py > @@ -19,6 +19,7 @@ > > import datetime > import itertools > +import smtplib > > from django.conf import settings > from django.contrib.auth.models import User > @@ -87,7 +88,7 @@ def send_notifications(): > > try: > message.send() > - except Exception as ex: > + except smtplib.SMTPException as ex: > errors.append((recipient, ex)) > continue > > diff --git a/patchwork/paginator.py b/patchwork/paginator.py > index e31c76c..3da2cd0 100644 > --- a/patchwork/paginator.py > +++ b/patchwork/paginator.py > @@ -55,9 +55,9 @@ class Paginator(paginator.Paginator): > super(Paginator, self).__init__(objects, items_per_page) > > try: > - page_no = int(request.GET.get('page')) > + page_no = int(request.GET.get('page', 1)) > self.current_page = self.page(int(page_no)) > - except Exception: > + except ValueError: > page_no = 1 > self.current_page = self.page(page_no) > > diff --git a/patchwork/templatetags/listurl.py b/patchwork/templatetags/listurl.py > index 3f28f71..eea788e 100644 > --- a/patchwork/templatetags/listurl.py > +++ b/patchwork/templatetags/listurl.py > @@ -63,9 +63,9 @@ class ListURLNode(template.defaulttags.URLNode): > > params = [] > try: > - qs_var = template.Variable('list_view.params') > - params = dict(qs_var.resolve(context)) > - except Exception: > + qs_var = template.Variable('list_view.params').resolve(context) > + params = dict(qs_var) > + except TypeError, template.VariableDoesNotExist: > pass > > for (k, v) in self.params.items(): > diff --git a/patchwork/views/__init__.py b/patchwork/views/__init__.py > index e1e40a8..98e451d 100644 > --- a/patchwork/views/__init__.py > +++ b/patchwork/views/__init__.py > @@ -173,13 +173,13 @@ def set_bundle(request, project, action, data, patches, context): > try: > bp = BundlePatch.objects.get(bundle=bundle, patch=patch) > bp.delete() > + except BundlePatch.DoesNotExist: > + pass > + else: > messages.success( > request, > "Patch '%s' removed from bundle %s\n" % (patch.name, > bundle.name)) > - # TODO(stephenfin): Make this less broad > - except Exception: > - pass > > bundle.save() > > diff --git a/patchwork/views/bundle.py b/patchwork/views/bundle.py > index ba569e2..e717429 100644 > --- a/patchwork/views/bundle.py > +++ b/patchwork/views/bundle.py > @@ -48,10 +48,7 @@ def setbundle(request): > patch_id = request.POST.get('patch_id', None) > if patch_id: > patch = get_object_or_404(Patch, id=patch_id) > - try: > - bundle.append_patch(patch) > - except Exception: > - pass > + bundle.append_patch(patch) > bundle.save() > elif action == 'add': > bundle = get_object_or_404(Bundle, > @@ -66,11 +63,8 @@ def setbundle(request): > patch_ids = get_patch_ids(request.POST) > > for patch_id in patch_ids: > - try: > - patch = Patch.objects.get(id=patch_id) > - bundle.append_patch(patch) > - except: > - pass > + patch = Patch.objects.get(id=patch_id) > + bundle.append_patch(patch) > > bundle.save() > elif action == 'delete': > @@ -78,11 +72,10 @@ def setbundle(request): > bundle = Bundle.objects.get(owner=request.user, > id=request.POST['id']) > bundle.delete() > - except Exception: > + except Bundle.DoesNotExist: > pass > > bundle = None > - > else: > bundle = get_object_or_404(Bundle, owner=request.user, > id=request.POST['bundle_id']) > diff --git a/patchwork/views/mail.py b/patchwork/views/mail.py > index 3695e5b..8744d79 100644 > --- a/patchwork/views/mail.py > +++ b/patchwork/views/mail.py > @@ -17,7 +17,7 @@ > # along with Patchwork; if not, write to the Free Software > # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > > -from __future__ import absolute_import > +import smtplib > > from django.conf import settings as conf_settings > from django.core.mail import send_mail > @@ -114,7 +114,7 @@ def _optinout(request, action, description): > send_mail('Patchwork %s confirmation' % description, mail, > conf_settings.DEFAULT_FROM_EMAIL, [email]) > context['email_sent'] = True > - except Exception: > + except smtplib.SMTPException: > context['error'] = ('An error occurred during confirmation . ' > 'Please try again later.') > context['admins'] = conf_settings.ADMINS > diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py > index 41a2ec8..e477572 100644 > --- a/patchwork/views/patch.py > +++ b/patchwork/views/patch.py > @@ -77,15 +77,15 @@ def patch(request, patch_id): > elif action == 'addtobundle': > bundle = get_object_or_404( > Bundle, id=request.POST.get('bundle_id')) > - try: > - bundle.append_patch(patch) > - bundle.save() > + if bundle.append_patch(patch) > messages.success(request, > - 'Patch added to bundle "%s"' % bundle.name) > - except Exception as ex: > + 'Patch "%s" added to bundle "%s"' % ( > + patch.name, bundle.name)) > + else: > messages.error(request, > - "Couldn't add patch '%s' to bundle %s: %s" > - % (patch.name, bundle.name, ex.message)) > + 'Failed to add patch "%s" to bundle "%s": ' > + 'patch is already in bundle' % ( > + patch.name, bundle.name)) > > # all other actions require edit privs > elif not editable: > diff --git a/patchwork/views/user.py b/patchwork/views/user.py > index dfbfde8..b82636f 100644 > --- a/patchwork/views/user.py > +++ b/patchwork/views/user.py > @@ -17,7 +17,7 @@ > # along with Patchwork; if not, write to the Free Software > # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > > -from __future__ import absolute_import > +import smtplib > > from django.contrib import auth > from django.contrib.auth.decorators import login_required > @@ -145,7 +145,7 @@ def link(request): > context, request=request), > settings.DEFAULT_FROM_EMAIL, > [form.cleaned_data['email']]) > - except Exception: > + except smtplib.SMTPException: > context['confirmation'] = None > context['error'] = ('An error occurred during confirmation. ' > 'Please try again later') > diff --git a/patchwork/views/xmlrpc.py b/patchwork/views/xmlrpc.py > index ea2a1e3..793e82a 100644 > --- a/patchwork/views/xmlrpc.py > +++ b/patchwork/views/xmlrpc.py > @@ -94,7 +94,7 @@ class PatchworkXMLRPCDispatcher(SimpleXMLRPCDispatcher, > try: > decoded = base64.b64decode(header.encode('ascii')).decode('ascii') > username, password = decoded.split(':', 1) > - except: > + except ValueError: > raise Exception('Invalid authentication credentials') > > return authenticate(username=username, password=password) > @@ -124,7 +124,7 @@ class PatchworkXMLRPCDispatcher(SimpleXMLRPCDispatcher, > response = self.dumps(response, methodresponse=1) > except six.moves.xmlrpc_client.Fault as fault: > response = self.dumps(fault) > - except: > + except Exception: # noqa > # report exception back to server > response = self.dumps( > six.moves.xmlrpc_client.Fault( > @@ -149,7 +149,7 @@ def xmlrpc(request): > if request.method == 'POST': > try: > ret = dispatcher._marshaled_dispatch(request) > - except Exception: > + except Exception: # noqa > return HttpResponseServerError() > else: > ret = dispatcher.generate_html_documentation() > -- > 2.7.4 > > _______________________________________________ > Patchwork mailing list > Patchwork@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/patchwork
On 23 Sep 10:54, Daniel Axtens wrote: > Hi Stephen, > > I've had a quick look at the patches labelled trivial. > > For those patches en bloc: > Reviewed-by: Daniel Axtens <dja@axtens.net> Thanks for the reviews. Merged.
diff --git a/patchwork/bin/pwclient b/patchwork/bin/pwclient index 791c511..b63db53 100755 --- a/patchwork/bin/pwclient +++ b/patchwork/bin/pwclient @@ -294,17 +294,10 @@ def action_get(rpc, patch_id): fname = "%s.%d" % (base_fname, i) i += 1 - try: - f = open(fname, "w") - except: - sys.stderr.write("Unable to open %s for writing\n" % fname) - sys.exit(1) - - try: + with open(fname, 'w') as f: f.write(unicode(s).encode("utf-8")) - f.close() print('Saved patch to %s' % fname) - except: + except IOError: sys.stderr.write("Failed to write to %s\n" % fname) sys.exit(1) diff --git a/patchwork/models.py b/patchwork/models.py index b1c6d8d..4c0aa88 100644 --- a/patchwork/models.py +++ b/patchwork/models.py @@ -580,12 +580,14 @@ class Bundle(models.Model): # see if the patch is already in this bundle if BundlePatch.objects.filter(bundle=self, patch=patch).count(): - raise Exception('patch is already in bundle') + return bp = BundlePatch.objects.create(bundle=self, patch=patch, order=max_order + 1) bp.save() + return bp + def public_url(self): if not self.public: return None diff --git a/patchwork/notifications.py b/patchwork/notifications.py index 5420401..16152ef 100644 --- a/patchwork/notifications.py +++ b/patchwork/notifications.py @@ -19,6 +19,7 @@ import datetime import itertools +import smtplib from django.conf import settings from django.contrib.auth.models import User @@ -87,7 +88,7 @@ def send_notifications(): try: message.send() - except Exception as ex: + except smtplib.SMTPException as ex: errors.append((recipient, ex)) continue diff --git a/patchwork/paginator.py b/patchwork/paginator.py index e31c76c..3da2cd0 100644 --- a/patchwork/paginator.py +++ b/patchwork/paginator.py @@ -55,9 +55,9 @@ class Paginator(paginator.Paginator): super(Paginator, self).__init__(objects, items_per_page) try: - page_no = int(request.GET.get('page')) + page_no = int(request.GET.get('page', 1)) self.current_page = self.page(int(page_no)) - except Exception: + except ValueError: page_no = 1 self.current_page = self.page(page_no) diff --git a/patchwork/templatetags/listurl.py b/patchwork/templatetags/listurl.py index 3f28f71..eea788e 100644 --- a/patchwork/templatetags/listurl.py +++ b/patchwork/templatetags/listurl.py @@ -63,9 +63,9 @@ class ListURLNode(template.defaulttags.URLNode): params = [] try: - qs_var = template.Variable('list_view.params') - params = dict(qs_var.resolve(context)) - except Exception: + qs_var = template.Variable('list_view.params').resolve(context) + params = dict(qs_var) + except TypeError, template.VariableDoesNotExist: pass for (k, v) in self.params.items(): diff --git a/patchwork/views/__init__.py b/patchwork/views/__init__.py index e1e40a8..98e451d 100644 --- a/patchwork/views/__init__.py +++ b/patchwork/views/__init__.py @@ -173,13 +173,13 @@ def set_bundle(request, project, action, data, patches, context): try: bp = BundlePatch.objects.get(bundle=bundle, patch=patch) bp.delete() + except BundlePatch.DoesNotExist: + pass + else: messages.success( request, "Patch '%s' removed from bundle %s\n" % (patch.name, bundle.name)) - # TODO(stephenfin): Make this less broad - except Exception: - pass bundle.save() diff --git a/patchwork/views/bundle.py b/patchwork/views/bundle.py index ba569e2..e717429 100644 --- a/patchwork/views/bundle.py +++ b/patchwork/views/bundle.py @@ -48,10 +48,7 @@ def setbundle(request): patch_id = request.POST.get('patch_id', None) if patch_id: patch = get_object_or_404(Patch, id=patch_id) - try: - bundle.append_patch(patch) - except Exception: - pass + bundle.append_patch(patch) bundle.save() elif action == 'add': bundle = get_object_or_404(Bundle, @@ -66,11 +63,8 @@ def setbundle(request): patch_ids = get_patch_ids(request.POST) for patch_id in patch_ids: - try: - patch = Patch.objects.get(id=patch_id) - bundle.append_patch(patch) - except: - pass + patch = Patch.objects.get(id=patch_id) + bundle.append_patch(patch) bundle.save() elif action == 'delete': @@ -78,11 +72,10 @@ def setbundle(request): bundle = Bundle.objects.get(owner=request.user, id=request.POST['id']) bundle.delete() - except Exception: + except Bundle.DoesNotExist: pass bundle = None - else: bundle = get_object_or_404(Bundle, owner=request.user, id=request.POST['bundle_id']) diff --git a/patchwork/views/mail.py b/patchwork/views/mail.py index 3695e5b..8744d79 100644 --- a/patchwork/views/mail.py +++ b/patchwork/views/mail.py @@ -17,7 +17,7 @@ # along with Patchwork; if not, write to the Free Software # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA -from __future__ import absolute_import +import smtplib from django.conf import settings as conf_settings from django.core.mail import send_mail @@ -114,7 +114,7 @@ def _optinout(request, action, description): send_mail('Patchwork %s confirmation' % description, mail, conf_settings.DEFAULT_FROM_EMAIL, [email]) context['email_sent'] = True - except Exception: + except smtplib.SMTPException: context['error'] = ('An error occurred during confirmation . ' 'Please try again later.') context['admins'] = conf_settings.ADMINS diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py index 41a2ec8..e477572 100644 --- a/patchwork/views/patch.py +++ b/patchwork/views/patch.py @@ -77,15 +77,15 @@ def patch(request, patch_id): elif action == 'addtobundle': bundle = get_object_or_404( Bundle, id=request.POST.get('bundle_id')) - try: - bundle.append_patch(patch) - bundle.save() + if bundle.append_patch(patch) messages.success(request, - 'Patch added to bundle "%s"' % bundle.name) - except Exception as ex: + 'Patch "%s" added to bundle "%s"' % ( + patch.name, bundle.name)) + else: messages.error(request, - "Couldn't add patch '%s' to bundle %s: %s" - % (patch.name, bundle.name, ex.message)) + 'Failed to add patch "%s" to bundle "%s": ' + 'patch is already in bundle' % ( + patch.name, bundle.name)) # all other actions require edit privs elif not editable: diff --git a/patchwork/views/user.py b/patchwork/views/user.py index dfbfde8..b82636f 100644 --- a/patchwork/views/user.py +++ b/patchwork/views/user.py @@ -17,7 +17,7 @@ # along with Patchwork; if not, write to the Free Software # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA -from __future__ import absolute_import +import smtplib from django.contrib import auth from django.contrib.auth.decorators import login_required @@ -145,7 +145,7 @@ def link(request): context, request=request), settings.DEFAULT_FROM_EMAIL, [form.cleaned_data['email']]) - except Exception: + except smtplib.SMTPException: context['confirmation'] = None context['error'] = ('An error occurred during confirmation. ' 'Please try again later') diff --git a/patchwork/views/xmlrpc.py b/patchwork/views/xmlrpc.py index ea2a1e3..793e82a 100644 --- a/patchwork/views/xmlrpc.py +++ b/patchwork/views/xmlrpc.py @@ -94,7 +94,7 @@ class PatchworkXMLRPCDispatcher(SimpleXMLRPCDispatcher, try: decoded = base64.b64decode(header.encode('ascii')).decode('ascii') username, password = decoded.split(':', 1) - except: + except ValueError: raise Exception('Invalid authentication credentials') return authenticate(username=username, password=password) @@ -124,7 +124,7 @@ class PatchworkXMLRPCDispatcher(SimpleXMLRPCDispatcher, response = self.dumps(response, methodresponse=1) except six.moves.xmlrpc_client.Fault as fault: response = self.dumps(fault) - except: + except Exception: # noqa # report exception back to server response = self.dumps( six.moves.xmlrpc_client.Fault( @@ -149,7 +149,7 @@ def xmlrpc(request): if request.method == 'POST': try: ret = dispatcher._marshaled_dispatch(request) - except Exception: + except Exception: # noqa return HttpResponseServerError() else: ret = dispatcher.generate_html_documentation()
The are two somewhat significant changes: * The behavior of 'Bundle.add_patch' is changed. Previously this would raise an exception if the provided patch already existed in the bundle. Since this code was only used in one location, change this to return the BundlePatch if valid else None and change the calling code to check return value instead of catching the exception. * Use a context manager to open the config file in pwclient. This loses a little granularity in error messaging, but this is a worthy compromise. Signed-off-by: Stephen Finucane <stephenfinucane@hotmail.com> --- patchwork/bin/pwclient | 11 ++--------- patchwork/models.py | 4 +++- patchwork/notifications.py | 3 ++- patchwork/paginator.py | 4 ++-- patchwork/templatetags/listurl.py | 6 +++--- patchwork/views/__init__.py | 6 +++--- patchwork/views/bundle.py | 15 ++++----------- patchwork/views/mail.py | 4 ++-- patchwork/views/patch.py | 14 +++++++------- patchwork/views/user.py | 4 ++-- patchwork/views/xmlrpc.py | 6 +++--- 11 files changed, 33 insertions(+), 44 deletions(-)