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

Check inconsistent return statements #711

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

r3m0t
Copy link

@r3m0t r3m0t commented Dec 2, 2017

This implements the same as #431 #527 (requested in #399). This is about the best that can be done without importing the ast module, and therefore this PR is much smaller.

PEP8:

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).

This:

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.

@r3m0t
Copy link
Author

r3m0t commented Dec 2, 2017

Things I'm not sure about:

  • Should it be disabled by default? It looks like it'll trip up on a lot of codebases (including, ironically, this one)
  • Should it support noqa?
  • Should it emit no more than one warning for a function?

Okay: def a():\n return 1\n return 2
Okay: def a():\n return
Okay: def a():\n return\n return
Okay: def a():\n def b():\n return\n return b
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These can be taken out as they are tested in the more detailed tests.

@sigmavirus24
Copy link
Member

So sorry for the delay on this @r3m0t:

Should it be disabled by default? It looks like it'll trip up on a lot of codebases (including, ironically, this one)

That makes me lean towards yes.

Should it support noqa?

I think not. Mostly because it will be disabled by default, it shouldn't be required here, plus once it's on and a code-base is passing, I somehow doubt folks will want to allow someone to # noqa it. We can always revise and add in later.

Should it emit no more than one warning for a function?

Eh. I don't think we have other checks in this codebase that behave that way so it's fine that this emits more than one per function.

@bz2
Copy link

bz2 commented Jan 30, 2019

@sigmavirus24 Can this just be landed then? Was looking for the pycodestyle code for this rule, and found it was added for pylint some time ago pylint-dev/pylint#1267 but never done here.

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

Successfully merging this pull request may close these issues.

3 participants