Added new rule for checking for infinite while loops. Added test call… #1066

ragohin · 2019-12-09T04:00:59Z

…ed test_infinite_while_loops.py

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

Closes #1007

sobolevn

Thanks! Great progress here. 👍

I have added some suggestions about your solution.

sobolevn · 2019-12-09T08:34:31Z

wemake_python_styleguide/visitors/ast/loops.py

@@ -203,6 +206,25 @@ def _check_multiline_loop(self, node: _AnyLoop) -> None:
                self.add_violation(MultilineLoopViolation(node))
                break

+    def _has_break_raise_or_return(self, node: _AnyLoop) -> bool:


This can be moved to logic/ dir

sobolevn · 2019-12-09T08:35:01Z

wemake_python_styleguide/visitors/ast/loops.py

+                    return True
+        return False
+
+    def _check_infinite_while_loop(self, node: _AnyLoop) -> None:


This can be moved to a another or a new visitor, that is ast.While specific.

Shouldn't this be inside of this class though, since it says at line 129 "Checks ''for'' and ''while'' loops" ? It seems like _check_multiline_loop also is ast.While specific.

I think what I have implemented as of now might work, but let me know!

coveralls · 2019-12-10T07:30:43Z

Pull Request Test Coverage Report for Build 2441

44 of 44 (100.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 2424:	0.0%
Covered Lines:	5041
Relevant Lines:	5041

💛 - Coveralls

sobolevn

Thanks a lot!

Now, let me show how integration test can be created:

def infinite_loop():
     while True:
          print('forever')

And put it into tests/fixtures/noqa.py with correct noqa comment.
Then adjust tests/test_checker/test_noqa.py to contain this violation.

That's it! Awesome job! 👍

sobolevn · 2019-12-10T09:54:03Z

wemake_python_styleguide/logic/nodes.py

+from wemake_python_styleguide.compat.aliases import ForNodes
+from wemake_python_styleguide.types import AnyFor, ContextNodes
+
+_AnyLoop = Union[AnyFor, ast.While]


Should go to types.py

sobolevn · 2019-12-10T09:54:19Z

wemake_python_styleguide/logic/nodes.py

@@ -29,3 +32,36 @@ def get_parent(node: ast.AST) -> Optional[ast.AST]:
 def get_context(node: ast.AST) -> Optional[ContextNodes]:
    """Returns the context or ``None`` if node has no context."""
    return getattr(node, 'wps_context', None)
+
+
+def does_loop_contain_node(  # TODO: move, reuse in annotations.py


Please, remove TODO

sobolevn · 2019-12-10T09:54:55Z

wemake_python_styleguide/logic/nodes.py

+    loop: Optional[_AnyLoop],
+    to_check: ast.Break,
+) -> bool:
+    """Helper function for has_break method."""


Please, change the description to something more meaningful.

sobolevn · 2019-12-10T09:55:21Z

wemake_python_styleguide/logic/nodes.py

@@ -29,3 +32,36 @@ def get_parent(node: ast.AST) -> Optional[ast.AST]:
 def get_context(node: ast.AST) -> Optional[ContextNodes]:
    """Returns the context or ``None`` if node has no context."""
    return getattr(node, 'wps_context', None)
+
+
+def does_loop_contain_node(  # TODO: move, reuse in annotations.py


Do we actually use this method from outside? If not, it should be protected. _does_loop_contain_node

sobolevn · 2019-12-10T09:56:19Z

wemake_python_styleguide/visitors/ast/loops.py

@@ -208,22 +178,21 @@ def _check_multiline_loop(self, node: _AnyLoop) -> None:

    def _has_break_raise_or_return(self, node: _AnyLoop) -> bool:


This method can still be moved into logic/

Sorry, that function should not have been in this commit in that location. It has been moved to logic/.

sobolevn · 2019-12-10T09:57:32Z

wemake_python_styleguide/visitors/ast/loops.py

@@ -338,6 +307,6 @@ def _check_implicit_items(self, node: ast.For) -> None:

    def _is_assigned_target(self, node: ast.Subscript) -> bool:
        parent = nodes.get_parent(node)
-        if not isinstance(parent, (*AssignNodes, ast.AugAssign)):


Are you sure about this change? Why do you have to remove ast.AugAssign?

Sorry, I don't remember changing that, I must've pulled an outdated version of the code. Fixed.

ragohin · 2019-12-10T19:15:20Z

I added one more small change so that "while True:" loops are defined with print("forever") inside. Hopefully everything is correct now!

sobolevn

Done! Only one little refactor remains.

Thanks a lot for your work!

sobolevn · 2019-12-10T20:00:12Z

tests/test_visitors/test_ast/test_loops/test_loops/test_infinite_while_loops.py

+from wemake_python_styleguide.visitors.ast.loops import WrongLoopVisitor
+
+incorrect_loop1 = """
+while True:


Looks like there are a lot of repetitive patterns in these samples.
Let's parametrize them!

Like so:

incorrect_loop_template = """ while True: {0} """

And later we can use this way of testing:

@pytest.mark.parametrize('keyword', [ 'break', 'return', 'return some', 'raise Some', 'raise Some()', 'raise', ] def test_whatever(keyword): code = template.format(keyword)

We also need to test a nested keyword:

while True: if some: {0}

Alright I think I fixed everything! Let me know what you think!

sobolevn · 2019-12-10T22:07:23Z

tests/test_visitors/test_ast/test_loops/test_loops/test_infinite_while_loops.py

+incorrect_loop_template_nonfunction3 = """
+while True:
+    if some:
+        print('forever')


This is not parametrized

How can I parametrize this in a way where {0} accepts any command except for (raise, return, and break)?

Basically my most recent pull request has (raise, return, break, etc) parametrized, but not "print('forever')" which should just be replaced by any command that is not (raise, return, break, etc).

ragohin · 2019-12-10T22:11:05Z

I think there is an issue with the configuration on the laptop I pushed my code with most recently.

I originally was working on my Mac, which just crashed, so I switched to wsl on my Windows computer. I think the code will build properly if I submit on my Mac, but I can't use it right now :/

Is there any work around with this because I'm not sure I can satisfy the Travis CI build with my current laptop?

sobolevn

At last! @ragohin now this is ready to be merged. I will include this feature into 0.15 version.

You PR was not polished, so I had to work on it for a bit more.
I hope that you will get your bonus credits for this work. Good luck!

Thanks a lot for taking the time to contribute to our project. I really appreciate that! 👍

codecov · 2020-10-20T14:18:41Z

Codecov Report

Merging #1066 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1066   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          105       105           
  Lines         5512      5531   +19     
  Branches      1217      1224    +7     
=========================================
+ Hits          5512      5531   +19

Impacted Files	Coverage Δ
wemake_python_styleguide/logic/tree/loops.py	`100.00% <100.00%> (ø)`
...ake_python_styleguide/violations/best_practices.py	`100.00% <100.00%> (ø)`
wemake_python_styleguide/visitors/ast/loops.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 033048a...0683566. Read the comment docs.

sobolevn requested changes Dec 9, 2019

View reviewed changes

sobolevn requested changes Dec 10, 2019

View reviewed changes

sobolevn reviewed Dec 10, 2019

View reviewed changes

ragohin requested a review from sobolevn December 10, 2019 22:26

sobolevn approved these changes Dec 11, 2019

View reviewed changes

sobolevn added the on hold label Dec 11, 2019

sobolevn added this to the Version 0.15 milestone Jan 19, 2020

sobolevn force-pushed the master branch from 4b1a202 to a0a009d Compare July 29, 2020 16:44

sobolevn added 2 commits October 20, 2020 16:58

Test

f3a6cb6

Rebase

2055a83

sobolevn force-pushed the issue-1007 branch from 79568ce to 2055a83 Compare October 20, 2020 14:03

sobolevn added 3 commits October 20, 2020 17:05

Rebase

1fa60e8

Fixes Ci

9d42bdd

Fixes Ci

0683566

sobolevn merged commit 34b9004 into wemake-services:master Oct 20, 2020

sobolevn modified the milestones: Version 0.16, Version 0.15 aka New runtime Oct 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added new rule for checking for infinite while loops. Added test call… #1066

Added new rule for checking for infinite while loops. Added test call… #1066

ragohin commented Dec 9, 2019 •

edited by sobolevn

sobolevn left a comment

sobolevn Dec 9, 2019

ragohin Dec 10, 2019

sobolevn Dec 9, 2019

ragohin Dec 10, 2019

ragohin Dec 10, 2019

coveralls commented Dec 10, 2019 •

edited

sobolevn left a comment •

edited

sobolevn Dec 10, 2019

ragohin Dec 10, 2019

sobolevn Dec 10, 2019

ragohin Dec 10, 2019

sobolevn Dec 10, 2019

ragohin Dec 10, 2019

sobolevn Dec 10, 2019

ragohin Dec 10, 2019

sobolevn Dec 10, 2019

ragohin Dec 10, 2019

sobolevn Dec 10, 2019

ragohin Dec 10, 2019

ragohin commented Dec 10, 2019

sobolevn left a comment

sobolevn Dec 10, 2019

ragohin Dec 10, 2019

sobolevn Dec 10, 2019

ragohin Dec 10, 2019

ragohin Dec 10, 2019

ragohin commented Dec 10, 2019

sobolevn left a comment •

edited

codecov bot commented Oct 20, 2020 •

edited

		@@ -208,22 +178,21 @@ def _check_multiline_loop(self, node: _AnyLoop) -> None:

		def _has_break_raise_or_return(self, node: _AnyLoop) -> bool:

Added new rule for checking for infinite while loops. Added test call… #1066

Added new rule for checking for infinite while loops. Added test call… #1066

Conversation

ragohin commented Dec 9, 2019 • edited by sobolevn

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

Choose a reason for hiding this comment

coveralls commented Dec 10, 2019 • edited

Pull Request Test Coverage Report for Build 2441

💛 - Coveralls

sobolevn left a comment • edited

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ragohin commented Dec 10, 2019

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

Choose a reason for hiding this comment

ragohin commented Dec 10, 2019

sobolevn left a comment • edited

Choose a reason for hiding this comment

codecov bot commented Oct 20, 2020 • edited

Codecov Report

ragohin commented Dec 9, 2019 •

edited by sobolevn

coveralls commented Dec 10, 2019 •

edited

sobolevn left a comment •

edited

sobolevn left a comment •

edited

codecov bot commented Oct 20, 2020 •

edited