Fix WPS430 false positive on whitelisted deep nested functions#3591
Fix WPS430 false positive on whitelisted deep nested functions#3591sobolevn merged 3 commits intowemake-services:masterfrom
WPS430 false positive on whitelisted deep nested functions#3591Conversation
|
@sobolevn hi! I opened a new PR with correct branch name and other edits |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3591 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 369 369
Lines 12297 12308 +11
Branches 849 850 +1
=========================================
+ Hits 12297 12308 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| def nested(): # noqa: WPS430 | ||
| ... | ||
|
|
||
| def factory(): # ok |
There was a problem hiding this comment.
No need to put this into noqa.py. Here we test that # noqa comments work correctly ;)
| 'WPS428': 0, # disabled since 1.0.0 | ||
| 'WPS429': 1, | ||
| 'WPS430': 1, | ||
| 'WPS430': 3, |
There was a problem hiding this comment.
Let's revert these changes.
| 'WPS430': 3, | |
| 'WPS430': 1, |
There was a problem hiding this comment.
I've added minimal reproducible example as part of integration tests (if I didn't misunderstood what CONTRIBUTING.md states)
Ok, sure, didn't catch that from the issue itself, thought that it is ok to define whitelisted functions regardless of nesting and what causes that nesting
Don't |
Nope, we modified code and none tests failed :) |
|
|
||
| assert_errors(visitor, [NestedFunctionViolation]) | ||
| assert_error_text(visitor, whitelist_name) | ||
| assert_errors(visitor, []) |
There was a problem hiding this comment.
Nope, we modified code and none tests failed :)
Not because I actually changed assertion in that case in this PR? As I meantioned before, this test assumed that all deep nested function definitions with whitelisted names should be reported as violation, regardless what causes that indentation
Did you mean that we just need exclude
into a separate test case that asserts there is no violation with if and if/else blocks, while keeping other deep nesting cases reported as violations?
Sorry for asking so many questions and not understanding what needed to be done from the first try :(
small edit added integration test case and updated CHANGELOG.md
1bbbe92 to
8711e66
Compare
| self.add_violation(NestedFunctionViolation(node, text=node.name)) | ||
| is_direct = isinstance(parent, FunctionNodes) | ||
|
|
||
| is_single_if = isinstance(parent, ast.If) and isinstance( |
There was a problem hiding this comment.
get_parent(parent) is get_context(node) that's the correct check :)
| get_parent(parent), FunctionNodes | ||
| ) | ||
|
|
||
| if node.name not in NESTED_FUNCTIONS_WHITELIST or not ( |
There was a problem hiding this comment.
like this?
if node.name not in NESTED_FUNCTIONS_WHITELIST:
self.add_violation(
NestedFunctionViolation(node, text=node.name),
)
return
if not is_direct and not is_single_if:
self.add_violation(
NestedFunctionViolation(node, text=node.name),
)There was a problem hiding this comment.
if node.name in NESTED_FUNCTIONS_WHITELIST and (is_direct or is_single_if): return :)
I have made things!
Checklist
CHANGELOG.mdRelated issues
Closes #3589
Actually, there was already a test case for this
/wemake-python-styleguide/tests/test_visitors/test_ast/test_complexity/test_nested/test_nested_functions.py, but it was wrong and assumed that deep nested whitelisted functions must be reported.