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

Conversation

blablatdinov
Copy link
Contributor

@blablatdinov blablatdinov commented Sep 6, 2022

I have made things!

Checklist

  • I have double checked that there are no unrelated changes in this pull request (old patches, accidental config files, etc)
  • I have created at least one test case for the changes I have made
  • I have updated the documentation for the changes I have made
  • I have added my changes to the CHANGELOG.md

Related issues

Format is:

🙏 Please, if you or your company is finding wemake-python-styleguide valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/wemake-python-styleguide. As a thank you, your profile/company logo will be added to our main README which receives hundreds of unique visitors per day.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Please, add configuration and default ratio (I think that default amount of empty lines should be 1-for-2 non-empty lines).

@blablatdinov
Copy link
Contributor Author

@sobolevn I don't agree or don't correctly understand you proportions. In this proportions linter don't reject next code:

def func(a, b):
    first_expression(a)

    second_expression(b)

Or you implied 2 line with expressions without gap between them?

@blablatdinov blablatdinov force-pushed the pr-restrict-empty-lines-in-func-body branch 3 times, most recently from e686c19 to a00ff60 Compare September 8, 2022 18:17
@sobolevn
Copy link
Member

sobolevn commented Sep 8, 2022

We can add a lower bound of 0 empty lines for 2 lines. That's not a problem.

@blablatdinov
Copy link
Contributor Author

@sobolevn I'm pushed commit with code for configurating this rule. Please, check it

@sobolevn
Copy link
Member

Can you please rebase your work? I've fixed new poetry@1.2.0 build

@blablatdinov blablatdinov force-pushed the pr-restrict-empty-lines-in-func-body branch from 58ece1e to 9e4943e Compare September 12, 2022 09:02
@blablatdinov
Copy link
Contributor Author

blablatdinov commented Sep 12, 2022

Yes, of course. My branch rebased now

@sobolevn
Copy link
Member

@blablatdinov blablatdinov force-pushed the pr-restrict-empty-lines-in-func-body branch from a30ad0b to cb34ca8 Compare September 12, 2022 09:42
@blablatdinov
Copy link
Contributor Author

blablatdinov commented Sep 12, 2022

Sorry, my bad. I connect my visitor and found a problem: ast module ignore comments, so my algorithm count it like empty line. I've been looked solution of this problem on Google and I can't find it (in stackoverflow recomends using tokenize module). Do you have any recomendation for resolving this problem ?

@blablatdinov blablatdinov force-pushed the pr-restrict-empty-lines-in-func-body branch 5 times, most recently from 8e7ba4c to 6f47f50 Compare September 12, 2022 17:40
@blablatdinov blablatdinov force-pushed the pr-restrict-empty-lines-in-func-body branch from 6f47f50 to 55ba64c Compare September 12, 2022 17:49
lighter()
"""

class_with_allow_method = """
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIP: what do you think about using valid instead of allow?
class_with_valid_method sounds better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'm agree. My english level is not high, so I'm happy with any edits

wemake_python_styleguide/options/config.py Outdated Show resolved Hide resolved
wemake_python_styleguide/options/defaults.py Outdated Show resolved Hide resolved
wemake_python_styleguide/violations/best_practices.py Outdated Show resolved Hide resolved
visitor = WrongEmptyLinesCountVisitor(default_options, tree=tree)
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

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

blablatdinov and others added 13 commits September 18, 2022 16:33
…es_count.py


fix typo

Co-authored-by: Łukasz Skarżyński <skarzynski_lukasz@protonmail.com>
fix typo

Co-authored-by: Łukasz Skarżyński <skarzynski_lukasz@protonmail.com>
fix typo

Co-authored-by: Łukasz Skarżyński <skarzynski_lukasz@protonmail.com>
Co-authored-by: Łukasz Skarżyński <skarzynski_lukasz@protonmail.com>
Co-authored-by: Łukasz Skarżyński <skarzynski_lukasz@protonmail.com>
Co-authored-by: Łukasz Skarżyński <skarzynski_lukasz@protonmail.com>
Co-authored-by: Łukasz Skarżyński <skarzynski_lukasz@protonmail.com>
Co-authored-by: Łukasz Skarżyński <skarzynski_lukasz@protonmail.com>
@blablatdinov
Copy link
Contributor Author

I'm change visitor to BaseTokenVisitor. Can you review it?

@codecov
Copy link

codecov bot commented Sep 25, 2022

Codecov Report

Merging #2487 (8f366b7) into master (5bfdbdd) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #2487   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          119       120    +1     
  Lines         6390      6431   +41     
  Branches      1449      1458    +9     
=========================================
+ Hits          6390      6431   +41     
Impacted Files Coverage Δ
wemake_python_styleguide/options/config.py 100.00% <ø> (ø)
wemake_python_styleguide/presets/types/tree.py 100.00% <ø> (ø)
wemake_python_styleguide/options/defaults.py 100.00% <100.00%> (ø)
wemake_python_styleguide/options/validation.py 100.00% <100.00%> (ø)
...ake_python_styleguide/violations/best_practices.py 100.00% <100.00%> (ø)
...on_styleguide/visitors/ast/function_empty_lines.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

"""


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

parse_tokens,
):
"""Testing wrong cases."""
file_tokens = parse_tokens(input_)
Copy link
Member

Choose a reason for hiding this comment

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

Don't forget to use mode fixture to test async def as well.

print(customer.phone)
# printing customer company
print(customer.company)
"""
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 👍

@blablatdinov
Copy link
Contributor Author

blablatdinov commented Sep 25, 2022

Thanks for comments. I'm add test cases with comments and docstrings. And wrapped input code snippets into mode fixture

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Awesome! This is the last piece of feedback that I have.

wemake_python_styleguide/violations/best_practices.py Outdated Show resolved Hide resolved
):
"""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

blablatdinov and others added 4 commits September 26, 2022 21:31
@blablatdinov
Copy link
Contributor Author

blablatdinov commented Sep 26, 2022

Thanks for your suggested changes! I added test with exps_for_one_empty_line=0 for method without empty lines

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thank you for the great idea and solid implementation!
👏

@sobolevn sobolevn merged commit 4ca670e into wemake-services:master Sep 26, 2022
@blablatdinov
Copy link
Contributor Author

Thank you too, this is my first contribution to open source with code! The "quality wall" in this repository is very high and strong, respect to the developers who build it. 🎉

I bought the course "typed python" from @f213 and @sobolevn, we'll meet there 😉

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.

Empty lines in method body
3 participants