Issue 1974 #1998

amansr02 · 2021-04-24T06:15:33Z

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.

skarzi

Thanks, that's PR with awesome changes, but it needs some polishing and few additional unit tests that check the newly introduced violation.
You can add a new test file to tests/test_visitors/test_ast/test_keywords/.
Let's check e.g. tests/test_visitors/test_ast/test_keywords/test_raise.py file with tests related to the raise statement (so you need to add quite similar ones).
If you have any questions etc, ping me here 😄

wemake_python_styleguide/logic/tree/operators.py

wemake_python_styleguide/violations/refactoring.py

wemake_python_styleguide/visitors/ast/keywords.py

tests/fixtures/noqa/noqa.py

wemake_python_styleguide/visitors/ast/keywords.py

tests/fixtures/noqa/noqa.py

…uide into issue-1974

amansr02 · 2021-04-25T06:01:08Z

Hi @skarzi. Thank you for reviewing our changes. They should be fixed now. We have integrated those changes and have added test cases. Flake8 runs perfectly on wemake-python-styleguide project. Do let us know if we need to change other things as well?

skarzi

great progress, just a few things are worth adding/refactoring, before shipping this feature!

tests/fixtures/noqa/noqa.py

tests/test_visitors/test_ast/test_keywords/test_raise.py

skarzi · 2021-04-25T14:22:45Z

tests/test_visitors/test_ast/test_keywords/test_raise.py

@@ -130,3 +131,22 @@ def test_bare_raise(
    visitor.run()

    assert_errors(visitor, [])
+
+
+def test_bare_raise_no_except(


let's add tests (or even better one parametrized test) for following cases:

bare raise inside except block

raise SomeException() inside except block

raise SomeException() not nested in except block

wemake_python_styleguide/logic/safe_eval.py

wemake_python_styleguide/visitors/ast/keywords.py

amansr02 · 2021-04-25T19:49:27Z

Hi! I have updated my changes and fixed one of the noqa test cases as well to reflect our added changes. Not sure if I was supposed to change the test_noqa function. Do let me know if this change works out! Greatly appreciate the feedback.

sobolevn

Thanks a lot!

sobolevn · 2021-04-26T05:58:29Z

tests/test_checker/test_noqa.py

@@ -442,7 +442,7 @@ def test_noqa_fixture(absolute_path):
    )
    stdout, _ = process.communicate()

-    assert stdout.count('WPS') == 0
+    assert stdout.count('WPS') == 1


This should not be changed 🙂

sobolevn · 2021-04-26T05:59:56Z

tests/test_visitors/test_ast/test_keywords/test_raise.py

+    default_options,
+):
+    """Testing that bare `raise` is not allowed without an except block."""
+    code = """


We tend to define module-level variables (like correct_case1 and correct_case2) and then use pytest.mark.parametrize to write a single test for all correct cases.

skarzi

great work, I just have added a few more comments and change requests/suggestions

skarzi · 2021-04-26T05:53:31Z

tests/fixtures/noqa/noqa.py

+
+def detect_bare_raise1():
+    """Function to check if the bare raise is detected."""
+    raise


Let's add noqa comment for the newly introduced violation and revert back test_noqa_fixture test in test_noqa.py

Suggested change

raise

raise # noqa: WPS532

skarzi · 2021-04-26T05:54:31Z

tests/test_checker/test_noqa.py

@@ -441,7 +442,7 @@ def test_noqa_fixture(absolute_path):
    )
    stdout, _ = process.communicate()

-    assert stdout.count('WPS') == 0
+    assert stdout.count('WPS') == 1


Suggested change

assert stdout.count('WPS') == 1

assert stdout.count('WPS') == 0

skarzi · 2021-04-26T06:00:01Z

tests/test_visitors/test_ast/test_keywords/test_raise.py

+
+def test_raise_except(
+    assert_errors,
+    parse_ast_tree,
+    default_options,
+):
+    """Testing that bare `raise` is not allowed without an except block."""
+    code = """
+    def hello():
+        try:
+            x = 1
+        except Exception:
+            raise ValueError('1')
+    """


Let's parametrize this test to test at least 2 cases:

raise SomeException() inside except block

bare raise inside except block

Suggested change

def test_raise_except(

assert_errors,

parse_ast_tree,

default_options,

):

"""Testing that bare `raise` is not allowed without an except block."""

code = """

def hello():

try:

x = 1

except Exception:

raise ValueError('1')

"""

@pytest.mark.parametrize('raise_statement', [

"raise ValueError('1')",

'raise',

])

def test_raise_inside_except(

assert_errors,

parse_ast_tree,

default_options,

raise_statement,

):

"""Ensure ``raise`` statement inside ``except`` block allowed."""

code = """

try:

x = 1

except Exception:

{raise_statement}

""".format(raise_statement=raise_statement)

You can also create one parametrized test for correct cases like suggested by Nikita in #1998 (comment) or me in #1998 (comment)

skarzi · 2021-04-26T06:00:39Z

tests/test_visitors/test_ast/test_keywords/test_raise.py

+    assert_errors(visitor, [])
+
+
+def test_raise_no_except(


Suggested change

def test_raise_no_except(

def test_raise_exception_no_except(

skarzi · 2021-04-26T06:01:21Z

tests/test_visitors/test_ast/test_keywords/test_raise.py

+    parse_ast_tree,
+    default_options,
+):
+    """Testing that bare `raise` is not allowed without an except block."""


Suggested change

"""Testing that bare `raise` is not allowed without an except block."""

"""Testing that `raise SomeException()` outside ``except`` block is allowed."""

codecov · 2021-04-26T10:22:13Z

Codecov Report

Merging #1998 (fc3db38) into master (b659768) will not change coverage.
The diff coverage is 100.00%.

❗ Current head fc3db38 differs from pull request most recent head e759a2d. Consider uploading reports for the commit e759a2d to get more accurate results

@@            Coverage Diff            @@
##            master     #1998   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          118       118           
  Lines         6208      6218   +10     
  Branches      1382      1385    +3     
=========================================
+ Hits          6208      6218   +10

Impacted Files	Coverage Δ
wemake_python_styleguide/violations/refactoring.py	`100.00% <100.00%> (ø)`
wemake_python_styleguide/visitors/ast/keywords.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 b659768...e759a2d. Read the comment docs.

amansr02 · 2021-04-26T17:43:57Z

I parameterized the test functions and now have 1 testcase that is correct and 1 testcase that raises an error in the linter.
Also updated the noqa.py. Thank you for the feedback guys! Really helping me become familiar with the codebase. Any other changes?

skarzi · 2021-04-27T07:33:39Z

tests/test_visitors/test_ast/test_keywords/test_raise.py

-    """
+    """Testing correct instances of `raise SomeException()` and bare `raise`."""
+    code = None
+    if bare_option:


personally, I dislike using if statements in tests, because they make tests harder to read and maintain, so I am suggesting to parameterized function with ready to parse code strings, e.g.:

@pytest.mark.parametrize('code', [ raise_exception_raw.format('ValueError(1)'), raise_exception_with_except_raw.format('ValueError(1)'), raise_exception_with_except_raw.format(''), ]) def test_correct raise_statement( assert_errors, parse_ast_tree, default_options, code, ): ...

skarzi · 2021-04-27T07:36:20Z

tests/test_visitors/test_ast/test_keywords/test_raise.py

+raise_exception_with_except = """
+def check_exception_without_call():
+    try:
+        x = 1
+    except Exception:
+        raise {0}


NIP: it's not necessary to wrap try except in function definition:

Suggested change

raise_exception_with_except = """

def check_exception_without_call():

try:

x = 1

except Exception:

raise {0}

raise_exception_with_except_raw = """

try:

x = 1

except Exception:

raise {0}

amansr02 and others added 8 commits April 20, 2021 00:45

added extra function. Need to add Violations

29a198b

fixed bare raise check and added new violation

3b9a0d4

fixed style in new code for bareRaise

0c783c0

noqa tests added and refactoring style fix

3640944

Merge branch 'master' into bareRaise

f180d08

fixed small error in refactoring.py

0431aeb

fixed refactoring

614387b

added to Changelog

7d416ae

skarzi requested changes Apr 24, 2021

View reviewed changes

amansr02 and others added 8 commits April 25, 2021 00:14

added a few changes mentioned in pull request review

557e9a4

changing parent to node in keywords.py

8a106f7

fixed refactoring.py and noqa flake8 errors

c849151

added test for bare raise without except

3afa362

Merge branch 'issue-1974' of github.com:amansr02/wemake-python-styleg…

eca2aee

…uide into issue-1974

added WPS532 to test_noqa.py

bd4f085

Merge branch 'issue-1974' of github.com:amansr02/wemake-python-styleg…

6ef4c6a

…uide into issue-1974

added version information

a84d0d1

skarzi requested changes Apr 25, 2021

View reviewed changes

fixed changes and updated tests for issue1974

fc3db38

sobolevn requested changes Apr 26, 2021

View reviewed changes

skarzi requested changes Apr 26, 2021

View reviewed changes

parametrized functions and updated noqa.py

e759a2d

amansr02 requested review from skarzi and sobolevn April 26, 2021 20:01

skarzi reviewed Apr 27, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 1974 #1998

Issue 1974 #1998

amansr02 commented Apr 24, 2021 •

edited

skarzi left a comment

amansr02 commented Apr 25, 2021

skarzi left a comment

skarzi Apr 25, 2021

amansr02 commented Apr 25, 2021

sobolevn left a comment

sobolevn Apr 26, 2021

sobolevn Apr 26, 2021

skarzi left a comment

skarzi Apr 26, 2021

skarzi Apr 26, 2021

skarzi Apr 26, 2021

skarzi Apr 26, 2021 •

edited

skarzi Apr 26, 2021

skarzi Apr 26, 2021

codecov bot commented Apr 26, 2021 •

edited

amansr02 commented Apr 26, 2021

skarzi Apr 27, 2021

skarzi Apr 27, 2021

	assert stdout.count('WPS') == 1
	assert stdout.count('WPS') == 0

	def test_raise_no_except(
	def test_raise_exception_no_except(

	"""Testing that bare `raise` is not allowed without an except block."""
	"""Testing that `raise SomeException()` outside ``except`` block is allowed."""

Issue 1974 #1998

Are you sure you want to change the base?

Issue 1974 #1998

Conversation

amansr02 commented Apr 24, 2021 • edited

I have made things!

Checklist

Related issues

skarzi left a comment

Choose a reason for hiding this comment

amansr02 commented Apr 25, 2021

skarzi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amansr02 commented Apr 25, 2021

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

skarzi 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

skarzi Apr 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 26, 2021 • edited

Codecov Report

amansr02 commented Apr 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amansr02 commented Apr 24, 2021 •

edited

skarzi Apr 26, 2021 •

edited

codecov bot commented Apr 26, 2021 •

edited