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

Limit empty lines in functions or methods body #2487

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
e48a050
add class and test for restrict all empty lines in func body
blablatdinov Sep 6, 2022
7136d32
add test case: function with comments
blablatdinov Sep 8, 2022
9bf523d
add configuration param
blablatdinov Sep 8, 2022
cf78379
fix baseline for formatting violation
blablatdinov Sep 8, 2022
8b490ab
self.generic_visit(node) in visitor
blablatdinov Sep 8, 2022
1715c0f
rename WrongEmptyLinesCountVisitorViolation -> WrongEmptyLinesCountVi…
blablatdinov Sep 8, 2022
9e4943e
0 available empty lines case
blablatdinov Sep 11, 2022
b11609c
update CHANGELOG
blablatdinov Sep 12, 2022
cb34ca8
Fix tests
blablatdinov Sep 12, 2022
55ba64c
Fix algorithm for calculating expression lines
blablatdinov Sep 12, 2022
f913f44
Update tests/test_visitors/test_ast/test_classes/test_wrong_empty_lin…
blablatdinov Sep 18, 2022
43a88b5
Update wemake_python_styleguide/options/config.py
blablatdinov Sep 18, 2022
88cdb63
Update wemake_python_styleguide/options/defaults.py
blablatdinov Sep 18, 2022
36b74d2
Update wemake_python_styleguide/violations/best_practices.py
blablatdinov Sep 18, 2022
d78bb99
Update wemake_python_styleguide/violations/best_practices.py
blablatdinov Sep 18, 2022
7a48d2f
Copying set by method
blablatdinov Sep 18, 2022
e409a41
Optimize visitor
blablatdinov Sep 18, 2022
2bfc662
Update wemake_python_styleguide/visitors/ast/function_empty_lines.py
blablatdinov Sep 18, 2022
57d8696
Fix typos and parse_ast_tree argument
blablatdinov Sep 18, 2022
e0370f9
Merge branch 'pr-restrict-empty-lines-in-func-body' of https://github…
blablatdinov Sep 18, 2022
4fda3be
change visitor to BaseTokenVisitor
blablatdinov Sep 25, 2022
b0cfd96
Merge branch 'master' into pr-restrict-empty-lines-in-func-body
blablatdinov Sep 25, 2022
3de1283
test option
blablatdinov Sep 25, 2022
919dc2a
test cases with docstring and comments
blablatdinov Sep 25, 2022
96bf0ee
fix error template
blablatdinov Sep 26, 2022
62ab4b9
expanded documentation
blablatdinov Sep 26, 2022
be93737
test valid configuration with allow zero empty lines
blablatdinov Sep 26, 2022
8f366b7
Merge branch 'pr-restrict-empty-lines-in-func-body' of https://github…
blablatdinov Sep 26, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ Semantic versioning in our case means:
- Adds `__init_subclass__` in the beginning of accepted methods
order as per WPS338 #2411

- Adds `WrongEmptyLinesCountViolation` #2486

### Bugfixes

- Fixes `WPS226` false positives on `|` use in `SomeType | AnotherType`
Expand Down
1 change: 1 addition & 0 deletions tests/test_checker/test_noqa.py
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,7 @@
'WPS613': 1,
'WPS614': 1,
'WPS615': 2,
'WPS473': 0,
})

# Violations which may be tweaked by `i_control_code` option:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,197 @@
"""Test method contain only allowed empty lines count."""
import pytest

from wemake_python_styleguide.violations.best_practices import (
WrongEmptyLinesCountViolation,
)
from wemake_python_styleguide.visitors.ast.function_empty_lines import (
WrongEmptyLinesCountVisitor,
)

class_with_wrong_method = """
class WrongClass(object):

def wrong_method(self):
foo()

bar()

baz()

lighter()
"""

class_with_valid_method = """
class WrongClass(object):

def wrong_method(self):
foo()
bar()
baz()
"""

wrong_function = """
def func():
foo()

a = 1 + 4

bar()

baz()
"""

wrong_function_with_loop = """
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about edge cases like below ones?

def foo():
    return (
        pekaboo['attr'] == 'dummy-value'
        and bar() == 'Knights that say Ni!'


        or (
            'attr2' in pekaboo
            and not some_object.attribute.method()
        )
    )
pekaboo(
    foo=123,



    bar='abc',
)

Can they be handled by the ast?
Probably this violation should be implemented by using BaseTokenVisitor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeh, I plan to change visitor to BaseTokenVisitor because ast ignore comments. I check these cases after changing the visitor, thank you

def func():
for x in range(10):

requests.get(


'https://github.com/wemake-services/wemake-python-styleguide'
)
"""

allow_function = """
def func():
foo()
if name == 'Moonflower':
print('Love')

baz()
"""

allow_function_with_comments = """
Copy link
Member

Choose a reason for hiding this comment

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

Please, add this case:

def test_func():
   """
   Its docstring

   has many new lines
   
   but this is 

   totally fine
 
   we don't raise a violation for this
   """
   return

def test_func():
# This function
#
# has lots
#
# of empty
#
# lines
#
# in comments
return 0
"""

function_with_docstring = """
def test_func():
\"""
Its docstring

has many new lines

but this is

totally fine

we don't raise a violation for this
\"""
return
"""

function_with_docstring_and_comments = """
def test_func():
\"""
Its docstring

has many new lines

but this is

totally fine

we don't raise a violation for this
\"""
# This function
#
# has lots
#
# of empty
#
# lines
#
# in comments
return 0
"""
Copy link
Member

Choose a reason for hiding this comment

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

We also need a test case like this:

def test_func():
   # This function 
   # 
   # has lots 
   #
   # of empty 
   #
   # lines 
   #
   # in comments
   return 0

Copy link
Member

Choose a reason for hiding this comment

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

A mix of comments and docstring is also a good idea 👍



@pytest.mark.parametrize('input_', [
class_with_wrong_method,
wrong_function,
wrong_function_with_loop,
])
def test_wrong(
input_,
default_options,
assert_errors,
parse_tokens,
mode,
):
"""Testing wrong cases."""
file_tokens = parse_tokens(mode(input_))

visitor = WrongEmptyLinesCountVisitor(
default_options, file_tokens=file_tokens,
)
visitor.run()

assert_errors(visitor, [WrongEmptyLinesCountViolation])


@pytest.mark.parametrize('input_', [
class_with_valid_method,
allow_function,
allow_function_with_comments,
function_with_docstring,
function_with_docstring_and_comments,
])
def test_success(
input_,
parse_tokens,
default_options,
assert_errors,
mode,
):
"""Testing available cases."""
file_tokens = parse_tokens(mode(input_))

visitor = WrongEmptyLinesCountVisitor(
default_options, file_tokens=file_tokens,
)
visitor.run()

assert_errors(visitor, [])


def test_zero_option(
parse_tokens,
default_options,
assert_errors,
options,
mode,
):
"""Test zero configuration."""
file_tokens = parse_tokens(mode(allow_function))
visitor = WrongEmptyLinesCountVisitor(
Copy link
Member

Choose a reason for hiding this comment

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

Let's also add a test that function without any empty lines do pass with exps_for_one_empty_line=0

options(exps_for_one_empty_line=0), file_tokens=file_tokens,
)
visitor.run()
assert_errors(visitor, [WrongEmptyLinesCountViolation])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this assertion correct? It seems like in this test input_ has only a valid number of empty lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to change parse_ast_tree after parameterizing the tests, thanks



def test_zero_option_with_valid_method(
parse_tokens,
default_options,
assert_errors,
options,
mode,
):
"""Test zero configuration with valid method."""
file_tokens = parse_tokens(mode(class_with_valid_method))
visitor = WrongEmptyLinesCountVisitor(
options(exps_for_one_empty_line=0), file_tokens=file_tokens,
)
visitor.run()
assert_errors(visitor, [])
8 changes: 8 additions & 0 deletions wemake_python_styleguide/options/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@
- ``forbidden-inline-ignore`` - list of codes of violations or
class of violations that are forbidden to ignore inline, defaults to
:str:`wemake_python_styleguide.options.defaults.FORBIDDEN_INLINE_IGNORE`
- ``exps-for-one-empty-line`` - number of expressions in
function or method body for available empty line (without code)
:str:`wemake_python_styleguide.options.defaults.EXPS_FOR_ONE_EMPTY_LINE`


.. rubric:: Complexity options
Expand Down Expand Up @@ -262,6 +265,11 @@ class Configuration(object):
type=String,
comma_separated_list=True,
),
_Option(
'--exps-for-one-empty-line',
defaults.EXPS_FOR_ONE_EMPTY_LINE,
'Count of expressions for one empty line in a function body.',
),

# Complexity:

Expand Down
3 changes: 3 additions & 0 deletions wemake_python_styleguide/options/defaults.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@
#: Violation codes that are forbidden to use.
FORBIDDEN_INLINE_IGNORE: Final = ()

#: Count of available expressions for one empty line in function or method body.
EXPS_FOR_ONE_EMPTY_LINE: Final = 2

# ===========
# Complexity:
# ===========
Expand Down
1 change: 1 addition & 0 deletions wemake_python_styleguide/options/validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ class _ValidatedOptions(object):
max_import_from_members: int = attr.ib(validator=[_min_max(min=1)])
max_tuple_unpack_length: int = attr.ib(validator=[_min_max(min=1)])
show_violation_links: bool
exps_for_one_empty_line: int


def validate_options(options: ConfigurationOptions) -> _ValidatedOptions:
Expand Down
3 changes: 3 additions & 0 deletions wemake_python_styleguide/presets/types/tree.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
conditions,
decorators,
exceptions,
function_empty_lines,
functions,
imports,
iterables,
Expand Down Expand Up @@ -107,6 +108,8 @@

redundancy.RedundantEnumerateVisitor,

function_empty_lines.WrongEmptyLinesCountVisitor,

# Modules:
modules.EmptyModuleContentsVisitor,
modules.MagicModuleFunctionsVisitor,
Expand Down
4 changes: 4 additions & 0 deletions wemake_python_styleguide/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -273,3 +273,7 @@ def max_tuple_unpack_length(self) -> int:
@property
def show_violation_links(self) -> bool:
...

@property
def exps_for_one_empty_line(self) -> int:
...
47 changes: 47 additions & 0 deletions wemake_python_styleguide/violations/best_practices.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
KwargsUnpackingInClassDefinitionViolation
ConsecutiveSlicesViolation
GettingElementByUnpackingViolation
WrongEmptyLinesCountViolation

Best practices
--------------
Expand Down Expand Up @@ -166,6 +167,7 @@
.. autoclass:: KwargsUnpackingInClassDefinitionViolation
.. autoclass:: ConsecutiveSlicesViolation
.. autoclass:: GettingElementByUnpackingViolation
.. autoclass:: WrongEmptyLinesCountViolation

"""

Expand Down Expand Up @@ -2770,3 +2772,48 @@ class GettingElementByUnpackingViolation(ASTViolation):
'Found unpacking used to get a single element from a collection'
)
code = 472


@final
class WrongEmptyLinesCountViolation(TokenizeViolation):
"""
Limit empty lines in functions or methods body.

Reasoning:
It's not holistic to have functions or methods that contain many
empty lines, and it makes sense to divide the method into several
ones.

Solution:
Limit count of empty lines of the function or method body
blablatdinov marked this conversation as resolved.
Show resolved Hide resolved
By default, we allow 1 empty line for 2 non-empty lines.

Example::

# Correct:
def func(name):
foo()
if name == 'Moonflower':
print('Love')
baz()

# Wrong:
def func(name):
foo()

if name == 'Moonflower':
print('Love')

baz()

Configuration:
This rule is configurable with ``--exps-for-one-empty-line``.
Default:
:str:`wemake_python_styleguide.options.defaults.EXPS_FOR_ONE_EMPTY_LINE`

.. versionadded:: 0.17.0

"""

error_template = 'Found too many empty lines in `def`: {0}'
code = 473
Loading