#21189: Correct the usage of bare except clauses in django's source
------------------------------+------------------------------------
Reporter: berkerpeksag | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+------------------------------------
Changes (by bmispelon):
* cc: bmispelon@… (added)
* needs_better_patch: => 0
* component: Template system => Core (Other)
* needs_tests: => 0
* needs_docs: => 0
* stage: Unreviewed => Accepted
Comment:
Hi,
The idea behind the bare `except` is that rendering should catch all
exceptions and, depending on the value of `settings.TEMPLATE_DEBUG`,
either output an empty string or re-raise it (it will be caught later down
the chain to display the familiar error page).
This is by design and it explains the test failures you get when you
restrict the types of exceptions being caught.
With that said, the usage of a bare `except` clause is usually a code
smell (and pep8 advises against it [1]). In this case for example, `except
Exception` might be better suited (`except Exception` catches all
exceptions except those like `SystemExit` which you generally don't want
to catch).
I did an audit of django's code to find the other instances of using bare
except clauses and I think there's room for improvement so I'll mark this
ticket as `accepted` but broaden its scope to cover other bare except
clauses.
The difficulty here lies in finding out which exception type(s) is
appropriate to catch (in general, while using `except Exception` is an
improvement over `except`, it's still too broad in most cases). This has
to be done on a case-by-case basis and usually require a good
understanding of the surrounding code.
So here's the list of places where I think `except` clauses should be
qualified.
Note that the line numbers are likely to get out of sync as time passes
(as I'm typing this, the current HEAD on master is at
c4fdd859ecba0b8e6dac6eca06429e66925722bd).
You can use `grep -rn except: django/` to find the occurences in your own
checkout.
*
[https://github.com/django/django/blob/master/django/template/loader_tags.py#L146
django/template/loader_tags.py:146]
*
[https://github.com/django/django/blob/master/django/core/management/sql.py#L66
django/core/management/sql.py:66]
*
[https://github.com/django/django/blob/master/django/core/management/__init__.py#L212
django/core/management/__init__.py:212]
*
[https://github.com/django/django/blob/master/django/core/management/__init__.py#L363
django/core/management/__init__.py:363]
*
[https://github.com/django/django/blob/master/django/core/mail/backends/smtp.py#L59
django/core/mail/backends/smtp.py:59]
*
[https://github.com/django/django/blob/master/django/core/mail/backends/smtp.py#L75
django/core/mail/backends/smtp.py:75]
*
[https://github.com/django/django/blob/master/django/core/mail/backends/smtp.py#L116
django/core/mail/backends/smtp.py:116]
*
[https://github.com/django/django/blob/master/django/core/mail/backends/console.py#L31
django/core/mail/backends/console.py:31]
*
[https://github.com/django/django/blob/master/django/http/request.py#L225
django/http/request.py:225]
*
[https://github.com/django/django/blob/master/django/http/multipartparser.py#L165
django/http/multipartparser.py:165]
*
[https://github.com/django/django/blob/master/django/http/multipartparser.py#L549
django/http/multipartparser.py:549]
*
[https://github.com/django/django/blob/master/django/http/multipartparser.py#L574
django/http/multipartparser.py:574]
*
[https://github.com/django/django/blob/master/django/db/backends/oracle/base.py#L620
django/db/backends/oracle/base.py:620]
*
[https://github.com/django/django/blob/master/django/contrib/gis/geoip/__init__.py#L17
django/contrib/gis/geoip/__init__.py:17]
*
[https://github.com/django/django/blob/master/django/contrib/gis/sitemaps/views.py#L83
django/contrib/gis/sitemaps/views.py:83]
*
[https://github.com/django/django/blob/master/django/contrib/gis/geos/libgeos.py#L68
django/contrib/gis/geos/libgeos.py:68]
*
[https://github.com/django/django/blob/master/django/contrib/gis/geos/libgeos.py#L78
django/contrib/gis/geos/libgeos.py:78]
*
[https://github.com/django/django/blob/master/django/contrib/gis/db/backends/base.py#L348
django/contrib/gis/db/backends/base.py:348]
*
[https://github.com/django/django/blob/master/django/contrib/gis/db/backends/spatialite/operations.py#L247
django/contrib/gis/db/backends/spatialite/operations.py:247]
*
[https://github.com/django/django/blob/master/django/contrib/gis/forms/fields.py#L76
django/contrib/gis/forms/fields.py:76]
*
[https://github.com/django/django/blob/master/django/contrib/gis/tests/inspectapp/tests.py#L142
django/contrib/gis/tests/inspectapp/tests.py:142]
*
[https://github.com/django/django/blob/master/django/contrib/gis/tests/layermap/tests.py#L153
django/contrib/gis/tests/layermap/tests.py:153]
*
[https://github.com/django/django/blob/master/django/contrib/gis/utils/layermapping.py#L340
django/contrib/gis/utils/layermapping.py:340]
*
[https://github.com/django/django/blob/master/django/contrib/gis/utils/layermapping.py#L368
django/contrib/gis/utils/layermapping.py:368]
*
[https://github.com/django/django/blob/master/django/contrib/gis/utils/__init__.py#L14
django/contrib/gis/utils/__init__.py:14]
There's also two instances where I'm not sure whether a bare `except` is
appropriate or not:
*
[https://github.com/django/django/blob/master/django/utils/module_loading.py#L60
django/utils/module_loading.py:60]
*
[https://github.com/django/django/blob/master/django/contrib/flatpages/middleware.py#L15
django/contrib/flatpages/middleware.py:15]
Finally, for completeness's sake, here are the instances of bare `except`
clauses which I believe are correct:
*
[https://github.com/django/django/blob/master/django/test/_doctest.py#L1321
django/test/_doctest.py:1321]
*
[https://github.com/django/django/blob/master/django/test/_doctest.py#L2631
django/test/_doctest.py:2631]
*
[https://github.com/django/django/blob/master/django/core/handlers/base.py#L153
django/core/handlers/base.py:153]
*
[https://github.com/django/django/blob/master/django/core/handlers/base.py#L167
django/core/handlers/base.py:167]
*
[https://github.com/django/django/blob/master/django/core/handlers/base.py#L183
django/core/handlers/base.py:183]
*
[https://github.com/django/django/blob/master/django/core/handlers/base.py#L193
django/core/handlers/base.py:193]
*
[https://github.com/django/django/blob/master/django/core/handlers/base.py#L203
django/core/handlers/base.py:203]
*
[https://github.com/django/django/blob/master/django/core/handlers/wsgi.py#L188
django/core/handlers/wsgi.py:188]
*
[https://github.com/django/django/blob/master/django/db/transaction.py#L485
django/db/transaction.py:485]
*
[https://github.com/django/django/blob/master/django/contrib/gis/utils/layermapping.py#L591
django/contrib/gis/utils/layermapping.py:591]
Of course, this doesn't have to be tackled all at once, so if anyone wants
to offer a patch for only one or two items in the list, I'd be happy to
review it (you also get bonus point if you can provided a testcase that
illustrates incorrect silencing of a particular type of exception).
Thanks for the report (and sorry for the big wall of text :) ).
[1] http://www.python.org/dev/peps/pep-0008/#programming-recommendations
--
Ticket URL: <https://code.djangoproject.com/ticket/21189#comment:1>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/070.18e8e1ea7fd4d111174aaab093cba135%40djangoproject.com.
For more options, visit https://groups.google.com/groups/opt_out.
