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

Detect super() call being used in the wrong context. #2530

Merged
merged 6 commits into from
Oct 15, 2022

Conversation

lensvol
Copy link
Collaborator

@lensvol lensvol commented Oct 14, 2022

Resolves #2310.

I have made things!

I've decided not to include the case of super() in a nested function due to the nature of the error there being fundamentally different from what is described in the bug report.

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

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

@lensvol lensvol requested a review from sobolevn October 14, 2022 21:50
@codecov
Copy link

codecov bot commented Oct 14, 2022

Codecov Report

Merging #2530 (da60ad0) into master (2f67026) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #2530   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          121       121           
  Lines         6477      6500   +23     
  Branches      1455      1462    +7     
=========================================
+ Hits          6477      6500   +23     
Impacted Files Coverage Δ
wemake_python_styleguide/presets/types/tree.py 100.00% <ø> (ø)
wemake_python_styleguide/violations/refactoring.py 100.00% <ø> (ø)
wemake_python_styleguide/violations/oop.py 100.00% <100.00%> (ø)
wemake_python_styleguide/visitors/ast/classes.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.

@lensvol
Copy link
Collaborator Author

lensvol commented Oct 15, 2022

@sobolevn I've implemented your suggestions, please review again.

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! Some final remarks :)

]
"""

error_nested_methods = """
Copy link
Member

Choose a reason for hiding this comment

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

We are still missing a correct test for this feature.

We need two:

  1. One with super(klass, self) (if it works in CPython)
  2. With regular non-nested super() call - just to be sure

Reasoning:
Call to `super()` without arguments will cause unexpected `TypeError`
in a number of specific contexts, e.g. dict/set/list comprehensions
and generator expressions.
Copy link
Member

Choose a reason for hiding this comment

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

and nested functions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will not amend that, as e.g. here indicates perfectly well that the list is incomplete.

@lensvol
Copy link
Collaborator Author

lensvol commented Oct 15, 2022

@sobolevn Added missing test case.

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!

@sobolevn sobolevn merged commit f55b180 into wemake-services:master Oct 15, 2022
@lensvol
Copy link
Collaborator Author

lensvol commented Oct 15, 2022

Glad I could help. Till next year!

P.S. Please do something about those pytest-based PR checks. It's already 2022, but contributing to this project still feels like walking on a field filled with rakes.

@sobolevn
Copy link
Member

Please do something about those pytest-based PR checks. It's already 2022, but contributing to this project still feels like walking on a field filled with rakes.

What do you mean?

@lensvol
Copy link
Collaborator Author

lensvol commented Oct 16, 2022

For example, test_all_violations_are_documented and test_violation_autoclass_order are completely unnecessary - you can write a simple script that will enumerate existing violations and automatically generate a comment that puts them in the correct autoclass order.

Yet right now they appear suddenly and force you to parse the output like this with your own eyes:

________________________________________________________________________ test_violation_autoclass_order ________________________________________________________________________

all_module_violations = {<module 'wemake_python_styleguide.violations.system' from '/Users/lensvol/projects/wemake-python-styleguide/wemake_py...PartialFloatViolation'>, <class 'wemake_python_styleguide.violations.consistency.FormattedStringViolation'>, ...], ...}

    def test_violation_autoclass_order(all_module_violations):
        """Used to force violations order inside the `autoclass` directives."""
        for module, classes in all_module_violations.items():
            sorted_by_code, _ = _get_sorted_classes(classes)
            pattern = re.compile(r'\.\.\sautoclass::\s(\w+)')
            sorted_by_autoclass = pattern.findall(module.__doc__)
            sorted_by_code = [cl.__qualname__ for cl in sorted_by_code]

>           assert sorted_by_code == sorted_by_autoclass
E           AssertionError: assert ['WrongMagicC...olation', ...] == ['WrongMagicC...olation', ...]
E             At index 3 diff: 'OveruseOfNoCoverCommentViolation' != 'ComplexDefaultValueViolation'
E             Left contains one more item: 'WrongEmptyLinesCountViolation'
E             Use -v to get more diff

tests/test_violations/test_definition_order.py:31: AssertionError
______________________________________________________________________ test_all_violations_are_documented ______________________________________________________________________

all_module_violations = {<module 'wemake_python_styleguide.violations.system' from '/Users/lensvol/projects/wemake-python-styleguide/wemake_py...PartialFloatViolation'>, <class 'wemake_python_styleguide.violations.consistency.FormattedStringViolation'>, ...], ...}

    def test_all_violations_are_documented(all_module_violations):
        """Ensures that all violations are documented."""
        for module, classes in all_module_violations.items():
            for violation_class in classes:
                # Once per `summary` and once per `autoclass`:
>               assert module.__doc__.count(violation_class.__qualname__) == 2
E               AssertionError: assert 1 == 2
E                +  where 1 = <built-in method count of str object at 0x7fc318924600>('OveruseOfNoCoverCommentViolation')
E                +    where <built-in method count of str object at 0x7fc318924600> = '\nThese checks ensure that you follow the best practices.\n\nThe source for these best practices is countless hours\n...veSlicesViolation\n.. autoclass:: GettingElementByUnpackingViolation\n.. autoclass:: WrongEmptyLinesCountViolation\n\n'.count
E                +      where '\nThese checks ensure that you follow the best practices.\n\nThe source for these best practices is countless hours\n...veSlicesViolation\n.. autoclass:: GettingElementByUnpackingViolation\n.. autoclass:: WrongEmptyLinesCountViolation\n\n' = <module 'wemake_python_styleguide.violations.best_practices' from '/Users/lensvol/projects/wemake-python-styleguide/wemake_python_styleguide/violations/best_practices.py'>.__doc__
E                +    and   'OveruseOfNoCoverCommentViolation' = <class 'wemake_python_styleguide.violations.best_practices.OveruseOfNoCoverCommentViolation'>.__qualname__

tests/test_violations/test_docs.py:13: AssertionError

And don't get me started on the noqa: checking using the test machine as well, this is a complete dumpster fire and misuse of the pytest. I can describe all this and propose solutions in a separate issue if you want.

@sobolevn
Copy link
Member

Yes, please!

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.

Do not allow super() without parameters in generator expressions
2 participants