Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consistent use of "return None" in functions #399

Open
gvanrossum opened this issue Apr 7, 2015 · 4 comments
Open

Consistent use of "return None" in functions #399

gvanrossum opened this issue Apr 7, 2015 · 4 comments

Comments

@gvanrossum
Copy link

Could you add this rule to pep8? I just updated the official PEP 8 to include this (I'm surprised it wasn't already there):

  • Be consistent in return statements. Either all return statements in a
    function should return an expression, or none of them should. If any return
    statement returns an expression, any return statements where no value is
    returned should explicitly state this as return None, and an explicit
    return statement should be present at the end of the function (if
    reachable).
   Yes:

   def foo(x):
       if x >= 0:
           return math.sqrt(x)
       else:
           return None

   def bar(x):
       if x < 0:
           return None
       return math.sqrt(x)

   No:

   def foo(x):
       if x >= 0:
           return math.sqrt(x)

   def bar(x):
       if x < 0:
           return
       return math.sqrt(x)

https://mail.python.org/pipermail/python-dev/2015-April/139054.html

@IanLee1521
Copy link
Member

Thanks, I saw this discussion and this helps remind me to implement it. :)

SylvainDe added a commit to SylvainDe/pep8 that referenced this issue Aug 14, 2015
First attempt to implement new check about
consistent returns. PEP 8 has been updated
accordingly (cf GvR message).

Implementation is still perfectible on many
points. Also, I've changed many details here
and there that are not relevant for the
issue. Finally, I'll need to double-check
the error number.
@asottile
Copy link
Member

fwiw, mypy is probably better suited to implement this type of check (and it already does):

import math

def foo(x: float) -> float:
   if x >= 0:
       return math.sqrt(x)

def bar(x: float) -> float:
   if x < 0:
       return
   return math.sqrt(x)
$ mypy --check-untyped-defs t.py
t.py:3: error: Missing return statement
t.py:9: error: Return value expected
Found 2 errors in 1 file (checked 1 source file)

@SylvainDe
Copy link

Quite a lot of work has been spent on this issue but not much actual progress what integrated.
Before trying to go further, it may be interesting to have answers for the following questions:

  1. do we still want to be handled by the pycodestyle tool ?

  2. if so: do we agree that it needs to be an AST-based check ? An alternative could be to perform the check is a slighty weaken form as in Check inconsistent return statements #711 which seems pretty interesting.

  3. If so, as far as I can tell: no other check built in pycodestyle is based on AST check which raise various questions:

  • should we ignore it by default to keep things fast by default ?

  • should we first make sure that AST-based check can be implemented in the code base without much trouble ? (ie: ensuring that all the test suite is valid Python code as much as possible ? also: how to handle tests corresponding to code that is parsed as valid Python code depending on the Python version)

These questions come from discussions over various PRs:

Having gone way too far is the AST check direction, I can see the troubles it leads to and think it may be just easier to consider that #711 is probably as good as things can be.

@sigmavirus24
Copy link
Member

I, for one, am of the mind that people should be using pylint in addition to flake8 and other tools. In light of the fact that pylint has support for this, is there urgency in us figuring out the right way to do this? For one thing, I'd rather as few false positives as possible. I wonder if, because this is an AST concern, if we can just write this as something that acts as an AST plugin for Flake8 (maybe one already exists?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants