Issue 3267: fixed wps226 location #3269

ishonichev · 2025-01-10T12:03:23Z

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

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

sobolevn

Thank you! We also need a test case for that. You can probably use ._location() in any of the existing tests.

sobolevn · 2025-01-10T14:22:12Z

wemake_python_styleguide/visitors/ast/complexity/overuses.py

@@ -67,6 +67,11 @@ def __init__(self, *args, **kwargs) -> None:
            int,
        ] = defaultdict(int)

+        self._string_constants_first_node: defaultdict[


This can be self._string_constants_first_node: ast.AST | None = None

Explain the idea, please

We only need the first node, right? We don't need all nodes. So, we can only save the first node here.

Yes. And it is done so. For each string their first nodes are collected in dict.

codecov · 2025-01-10T14:29:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (11c3633) to head (4581556).
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master     #3269   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          357       357           
  Lines        11703     11715   +12     
  Branches       801       802    +1     
=========================================
+ Hits         11703     11715   +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sobolevn · 2025-01-11T06:28:08Z

tests/test_visitors/conftest.py

+        if ignored_types:
+            real_errors = [
+                error
+                for error in visitor.violations
+                if not isinstance(error, ignored_types)
+            ]
+        else:
+            real_errors = visitor.violations
+
+        for violation in real_errors:
+            assert violation._location() != (0, 0)


Suggested change

if ignored_types:

real_errors = [

error

for error in visitor.violations

if not isinstance(error, ignored_types)

]

else:

real_errors = visitor.violations

for violation in real_errors:

assert violation._location() != (0, 0)

assert len(visitor.violations) == 1

violation = visitor.violations[0]

assert violation._location() == expected

sobolevn · 2025-01-11T06:28:24Z

tests/test_visitors/conftest.py

+    def factory(
+        visitor: BaseVisitor,
+        ignored_types: _IgnoredTypes = None,
+    ) -> None:


Suggested change

def factory(

visitor: BaseVisitor,

ignored_types: _IgnoredTypes = None,

) -> None:

def factory(

visitor: BaseVisitor,

expected: tuple[int, int],

) -> None:

sobolevn · 2025-01-11T06:28:51Z

tests/test_visitors/test_ast/test_complexity/test_overuses/test_overused_string.py

@@ -177,6 +178,7 @@ def test_string_overuse(
        string_value.replace('"', '') or "''",
        default_options.max_string_usages,
    )
+    assert_violation_location(visitor)


assert_violation_location(visitor, (X, Y))

sobolevn · 2025-01-11T06:29:38Z

wemake_python_styleguide/visitors/ast/complexity/overuses.py

@@ -67,6 +67,11 @@ def __init__(self, *args, **kwargs) -> None:
            int,
        ] = defaultdict(int)

+        self._string_constants_first_node: defaultdict[


We only need the first node, right? We don't need all nodes. So, we can only save the first node here.

ishonichev · 2025-01-12T13:25:35Z

Problem is slightly more trickier than I thought.
The reason is that OverusedStringViolation inherits from MaybeASTViolation which is ...violation without explicit node.
Also in assert_errors only for instances of ASTViolation or TokenizeViolation location is implicitly asserted (is compared with (0,0)).
For any type of violation its location isn't asserted explicitly (is compared with expected location), is it?

sobolevn · 2025-01-12T15:17:16Z

tests/test_visitors/test_ast/test_complexity/test_overuses/test_overused_string.py

@@ -178,7 +181,7 @@ def test_string_overuse(
        string_value.replace('"', '') or "''",
        default_options.max_string_usages,
    )
-    assert_violation_location(visitor)
+    assert_error_location(visitor, expected_location)


Can we just create a single test with known location? Like x = 'abc' and have it as a constant? Like (1, 4)? Right now it seems too complex.

sobolevn

Great work! Thank you!

ishonichev changed the title ~~fixed wps226 location~~ Issue 3267: fixed wps226 location Jan 10, 2025

sobolevn reviewed Jan 10, 2025

View reviewed changes

ishonichev force-pushed the master branch from f6470c5 to 1a2d487 Compare January 10, 2025 22:21

ishonichev marked this pull request as draft January 10, 2025 22:22

sobolevn reviewed Jan 11, 2025

View reviewed changes

ishonichev marked this pull request as ready for review January 12, 2025 13:26

sobolevn reviewed Jan 12, 2025

View reviewed changes

sobolevn mentioned this pull request Jan 15, 2025

Fix Issue 3267 #3279

Closed

4 tasks

fix wps226 location

07d6a63

ishonichev force-pushed the master branch from eca5ddf to 07d6a63 Compare February 5, 2025 18:50

Merge branch 'master' into master

4581556

sobolevn approved these changes Feb 6, 2025

View reviewed changes

sobolevn merged commit bae29e5 into wemake-services:master Feb 6, 2025
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 3267: fixed wps226 location #3269

Issue 3267: fixed wps226 location #3269

ishonichev commented Jan 10, 2025 •

edited

Loading

sobolevn left a comment

sobolevn Jan 10, 2025

ishonichev Jan 10, 2025

sobolevn Jan 11, 2025

ishonichev Jan 12, 2025

codecov bot commented Jan 10, 2025 •

edited

Loading

sobolevn Jan 11, 2025

sobolevn Jan 11, 2025

sobolevn Jan 11, 2025

sobolevn Jan 11, 2025

ishonichev commented Jan 12, 2025

sobolevn Jan 12, 2025

sobolevn left a comment

Issue 3267: fixed wps226 location #3269

Issue 3267: fixed wps226 location #3269

Conversation

ishonichev commented Jan 10, 2025 • edited Loading

I have made things!

Checklist

Related issues

sobolevn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 10, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishonichev commented Jan 12, 2025

Choose a reason for hiding this comment

sobolevn left a comment

Choose a reason for hiding this comment

ishonichev commented Jan 10, 2025 •

edited

Loading

codecov bot commented Jan 10, 2025 •

edited

Loading