Early return should not take return None form when it is not required

[sobolevn]

Rule request

Thesis

Right now this code:

    def _check_parent(self, node: ast.If) -> None:
        parent = get_parent(node)

        if parent is None:
            return None

        if isinstance(parent, _ELSE_NODES):
            body = parent.body + parent.orelse
        else:
            body = getattr(parent, 'body', [node])

        next_index_in_parent = body.index(node) + 1
        if keywords.next_node_returns_bool(body, next_index_in_parent):
            self.add_violation(SimplifiableReturningIfViolation(node))

passes our style check. Note return None part there. It is the only return. And it means that None part is not required.
Moreover, it should not be allowed. Simple early returns must be just return`.

[siddharth1704]

so we need just change that from return None to return

[sobolevn]

@siddharth1704 yes!

I think that we can formulate this rule as:

If you have a single return None, it can only be the last item in a function's body.

[cdhiraj40]

@sobolevn if we have returned none in a function, it means it's meant for later use, since we don't have the value to return, we do return none? please correct me if I'm wrong.

[cdhiraj40]

@sobolevn if we have returned none in a function, it means it's meant for later use, since we don't have the value to return, we do return none? please correct me if I'm wrong.

i would like to contribute to this, please assign me.

[sobolevn]

We allow:

def some():
   if condition:
       return None
   return other

We don't allow:

def some():
   if condition:
       return None
   print()

[cdhiraj40]

@sobolevn can you tell me the process to contribute?
i am a beginner and i didn't understand much from contribute.md. thanks!

[sobolevn]

You should probably start with learning what ast is. Then, look at

wemake-python-styleguide/wemake_python_styleguide/visitors/ast/keywords.py

Line 80 in e535aed

class ConsistentReturningVisitor(BaseNodeVisitor):

This change should leave somewhere there.

[cdhiraj40]

hey @sobolevn I understood the concept(thesis) and I did comprehend how ast works but I'm still not able to get how to implement
can you provide me a sample code from some other issue that is easy to understand
will be really helpful for a beginner like me, Thanks!

[sobolevn]

Ok, we also need to take a closer look at:

def some_funct():
    if first_cond:
        return None
    if second_cond:
        return None
    print('a')

I would re-formulate the rule as: if all function return nodes are return None -> than we must use plain return.

@DhirajChauhan40
Something like (untested):

is_all_none = (
            issubclass(returning_type, ast.Return) and
            has_values and
            all(
                ret_node.value.value is None
                for ret_node in return_nodes
                if isinstance(ret_node.value, (compat.nodes.Constant, ast.NameConstant))
            )
        )
        if is_all_none:
            self.add_violation(...)

You can put it here:

wemake-python-styleguide/wemake_python_styleguide/visitors/ast/keywords.py

Line 123 in d41d4b9

You would still have to:

1. Add new violation type in violations/consistency.py
2. Add tests
3. Have fun 🙂

[cdhiraj40]

@sobolevn
thank you so much!!

[cdhiraj40]

@sobolevn so i was tryna run the file and I got this error in confest.py inside tests folder

ImportError while loading conftest '/home/thefunnyintrovert/wemake-python-styleguide/tests/conftest.py'.
  File "/home/thefunnyintrovert/wemake-python-styleguide/tests/conftest.py", line 20
    def factory(*files: str):
                      ^
SyntaxError: invalid syntax

[sobolevn]

Are you running pythno3.9?

[cdhiraj40]

Are you running pythno3.9?

yes after installing the 3.9 version, it was working thanks!
sorry for the late reply i had work, i have written the test cases and now im working on the post part.

[cdhiraj40]

Hey @sobolevn i have added my violation to violations/consistency.py and have made other changes too

from wemake_python_styleguide.violations.consistency import (
    AllReturnsAreNoneViolation,

but In test_consistency_return.py I'm still getting module error

 from wemake_python_styleguide.violations.consistency import (
ImportError: cannot import name 'AllReturnsAreNoneViolation' from 'wemake_python_styleguide.violations.consistency'

[sobolevn]

Can you please open a PR?

[cdhiraj40]

yes sure

[sathieu]

How should I write the following code without noqa?

    def files_key(self, package_file: PackageFile) -> Optional[str]:  # noqa: WPS324
        """Get files key, to upload to. If None, uploaded as body.

        Args:
            package_file: Source package file.
        Returns:
            The files key, or None.
        """
        return None  # noqa: WPS324

This function is overriden in child classes.

NB: Using return (without None) fails with mypy:

mypy run-test: commands[0] | mypy gitlabracadabra
gitlabracadabra/packages/destination.py:160: error: Return value expected

See https://gitlab.com/gitlabracadabra/gitlabracadabra/-/merge_requests/233

[sobolevn]

def files_key(self, package_file: PackageFile) -> Optional[str]:  # noqa: WPS324
        """Get files key, to upload to. If None, uploaded as body.

        Args:
            package_file: Source package file.
        Returns:
            The files key, or None.
        """

🙂

[sathieu]

@sobolevn Now I have:

  157:1    DAR202 Excess "Returns" in Docstring: + return
  """Get files key, to upload to. If None, uploaded as body.

        Args:
            package_file: Source package file.

        Returns:
            The files key, or None.
        """

[sobolevn]

Hm, can you please open a new bug? Looks like when we have Returns: spec, we have a false positive.

[sathieu]

@sobolevn I created #2323.