[PATCH 05/13] trivial: Remove broad exceptions where possible
Daniel Axtens
dja at axtens.net
Fri Sep 23 10:54:41 AEST 2016
Hi Stephen,
I've had a quick look at the patches labelled trivial.
For those patches en bloc:
Reviewed-by: Daniel Axtens <dja at axtens.net>
Regards,
Daniel
Stephen Finucane <stephenfinucane at 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 at 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 at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 859 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/patchwork/attachments/20160923/f3e9d496/attachment.sig>
More information about the Patchwork
mailing list