[Django] #28858: Remove 'else' after 'return' or 'raise'
Django
------------------------------------------------+------------------------
Reporter: Дилян Палаузов | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Uncategorized | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
... as it has no added value.
--
Ticket URL: <https://code.djangoproject.com/ticket/28858>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Дилян Палаузов | Owner: nobody
Cleanup/optimization |
Component: Uncategorized | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Дилян Палаузов):
* Attachment "no-else-after-return.patch" added.
Django
Reporter: Дилян Палаузов | Owner: nobody
Cleanup/optimization |
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* component: Uncategorized => Core (Other)
Comment:
Personally, I think it has some readability benefits, at least in some
places.
--
Ticket URL: <https://code.djangoproject.com/ticket/28858#comment:1>
Django
Reporter: Дилян Палаузов | Owner: nobody
Cleanup/optimization |
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tomer Chachamu):
* cc: Tomer Chachamu (added)
Comment:
The generated CPython bytecode is usually the same, I see no benefit in
applying this rule.
--
Ticket URL: <https://code.djangoproject.com/ticket/28858#comment:2>
Django
Reporter: Дилян Палаузов | Owner: nobody
Cleanup/optimization |
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Дилян Палаузов):
Skipping {{{else}}} after {{{return}}}, when it does not impact
readability, leads to less code, and the resulting code is faster to read
(by humans).
Currently it is unclear, when useless {{{else}}} shall be written.
--
Ticket URL: <https://code.djangoproject.com/ticket/28858#comment:3>
Django
Reporter: Дилян Палаузов | Owner: nobody
Cleanup/optimization |
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tomer Chachamu):
I think you answered your own question there - it is used when it impacts
readability (as judged by the original authors, who may not always agree
with each other).
An `if..elif..else..` block clearly lists mutually exclusive branches
taken, and can be seen by looking at a single indentation level.
`if..return; if.. return;..` requires checking two indentation levels to
ensure there's a `return/raise` ending every `if`.
For example, in the first hunk of the patch, the code is:
{{{
if bla:
some code
if bla2:
raise
elif bla3:
raise
return val
}}}
After the change, it is:
{{{
if bla:
some code
if bla2:
raise
if bla3:
raise
return val
}}}
A newline before `if bla2` would improve readability, however the `elif`
is already helping to show there is some similarity between `if bla2` and
`if bla3`, and that `if bla` has a different purpose.
--
Ticket URL: <https://code.djangoproject.com/ticket/28858#comment:4>
Django
Reporter: Дилян Палаузов | Owner: nobody
Cleanup/optimization |
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Дилян Палаузов):
python has no switch..case, but "elif", which is not the same. The
readability criterion is very subjective one, e.g. the author might not
have been aware that the 'else' was useless. As for
{{{
if x:
return y
else:
raise z
}}}
the {{{else}}} does not add for readability.
The first snippets of django/contrib/gis/gdal/srs.py or
django/contrib/gis/geoip2/base.py I sent, have nothing to do with
readability.
--
Ticket URL: <https://code.djangoproject.com/ticket/28858#comment:5>
Django
Reporter: Дилян Палаузов | Owner: nobody
Cleanup/optimization |
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam (Chainz) Johnson):
So I'm very slightly in favour of removing the useless 'elses', I also
consider it a tiny bit more readable, and amazingly cpython *does*
generate less code in this case (it's no faster though, it's just removing
an extra two opcodes from a redundant return None):
{{{
In [6]: def foo():
...: if foo:
...: return 1
...: else:
...: return 2
...:
In [7]: def bar():
...: if bar:
...: return 1
...: return 2
...:
In [8]: dis.dis(foo)
2 0 LOAD_GLOBAL 0 (foo)
2 POP_JUMP_IF_FALSE 8
3 4 LOAD_CONST 1 (1)
6 RETURN_VALUE
5 >> 8 LOAD_CONST 2 (2)
10 RETURN_VALUE
12 LOAD_CONST 0 (None)
14 RETURN_VALUE
In [9]: dis.dis(bar)
2 0 LOAD_GLOBAL 0 (bar)
2 POP_JUMP_IF_FALSE 8
3 4 LOAD_CONST 1 (1)
6 RETURN_VALUE
4 >> 8 LOAD_CONST 2 (2)
10 RETURN_VALUE
}}}
However I doubt it's worth anyone's time to go through Django removing
them. Perhaps we can add it to the style guide now, close this ticket, and
fix it as code gets modified in the future.
--
Ticket URL: <https://code.djangoproject.com/ticket/28858#comment:6>
Django
Reporter: Дилян Палаузов | Owner: nobody
Cleanup/optimization |
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Unreviewed => Someday/Maybe
Comment:
The reporter already provided a patch and I created a
[https://github.com/django/django/pull/9498 PR]. Opinions welcome.
--
Ticket URL: <https://code.djangoproject.com/ticket/28858#comment:7>
Django
Reporter: Дилян Палаузов | Owner: nobody
Cleanup/optimization |
Component: Core (Other) | Version: master
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: new => closed
* resolution: => wontfix
Comment:
The patch has gone stale and the discussion on the PR didn't yield a
strong consensus. I think the value of trying to enforce some guidelines
around this style is minimal.
--
Ticket URL: <https://code.djangoproject.com/ticket/28858#comment:8>