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

Issue 1974 #1998

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ Semantic versioning in our case means:
### Bugfixes

- Fixes `UselessReturningElseViolation` to not report `else` with `break` #1958
- Fixes bare raise outside of except block #1974

### Misc

Expand Down
17 changes: 17 additions & 0 deletions tests/fixtures/noqa/noqa.py
Original file line number Diff line number Diff line change
Expand Up @@ -788,3 +788,20 @@ def get_item(): # noqa: WPS463
some(number) for numbers in matrix
for number in numbers # noqa: WPS361
]

def detect_bare_raise1():
skarzi marked this conversation as resolved.
Show resolved Hide resolved
"""Function to check if the bare raise is detected."""
raise
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


def detect_bare_raise2_helper():
"""Function to check if the bare raise is detected."""
raise ZeroDivisionError

def detect_bare_raise2():
"""Function to check if the bare raise is detected."""
try:
testvar = 1/0 # noqa: WPS344, F841
except ZeroDivisionError:
detect_bare_raise2()
raise ZeroDivisionError

skarzi marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions tests/test_checker/test_noqa.py
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,7 @@
'WPS529': 1,
'WPS530': 1,
'WPS531': 1,
'WPS532': 1,

'WPS600': 1,
'WPS601': 1,
Expand Down
20 changes: 20 additions & 0 deletions tests/test_visitors/test_ast/test_keywords/test_raise.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
BaseExceptionRaiseViolation,
RaiseNotImplementedViolation,
)
from wemake_python_styleguide.violations.refactoring import BareRaiseViolation
from wemake_python_styleguide.visitors.ast.keywords import WrongRaiseVisitor

raise_exception_method = """
Expand Down Expand Up @@ -130,3 +131,22 @@ def test_bare_raise(
visitor.run()

assert_errors(visitor, [])


def test_bare_raise_no_except(
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

assert_errors,
parse_ast_tree,
default_options,
):
"""Testing that bare `raise` is not allowed without an except block."""
code = """
def hello():
raise
hello()
skarzi marked this conversation as resolved.
Show resolved Hide resolved
"""
tree = parse_ast_tree(code)

visitor = WrongRaiseVisitor(default_options, tree=tree)
visitor.run()

assert_errors(visitor, [BareRaiseViolation])
1 change: 0 additions & 1 deletion wemake_python_styleguide/logic/safe_eval.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ def _convert_num(node: Optional[ast.AST]):
elif isinstance(node, ast.Name):
# We return string names as is, see how we return strings:
return node.id
raise ValueError('malformed node or string: {0!r}'.format(node))
skarzi marked this conversation as resolved.
Show resolved Hide resolved


def _convert_signed_num(node: Optional[ast.AST]):
Expand Down
37 changes: 36 additions & 1 deletion wemake_python_styleguide/violations/refactoring.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
ImplicitDictGetViolation
ImplicitNegativeIndexViolation
SimplifiableReturningIfViolation
BareRaiseViolation

Refactoring opportunities
-------------------------
Expand Down Expand Up @@ -81,7 +82,7 @@
.. autoclass:: ImplicitDictGetViolation
.. autoclass:: ImplicitNegativeIndexViolation
.. autoclass:: SimplifiableReturningIfViolation

.. autoclass:: BareRaiseViolation
"""

from typing_extensions import final
Expand Down Expand Up @@ -1233,3 +1234,37 @@ def some_function():

error_template = 'Found simplifiable returning `if` condition in a function'
code = 531


@final
class BareRaiseViolation(ASTViolation):
"""
Forbid bare ``raise`` outside of ``except`` block.

Reasoning:
One could call a function from an ``except`` and have
a bare ``raise`` inside but it is considered bad practice
to have a bare ``raise`` outside of an ``except`` block.

Solution:
Use a ``raise`` statement inside a parent ``except`` block.

Example::

# good
def smth():
try:
...
except:
raise

# bad
def smth():
raise

.. versionadded:: 0.15.3

"""

error_template = 'Bare raise outside of except block detected'
code = 532
8 changes: 8 additions & 0 deletions wemake_python_styleguide/visitors/ast/keywords.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
IncorrectYieldFromTargetViolation,
MultipleContextManagerAssignmentsViolation,
)
from wemake_python_styleguide.violations.refactoring import BareRaiseViolation
from wemake_python_styleguide.visitors.base import BaseNodeVisitor
from wemake_python_styleguide.visitors.decorators import alias

Expand All @@ -57,9 +58,11 @@ def visit_Raise(self, node: ast.Raise) -> None:

Raises:
RaiseNotImplementedViolation
BareRaiseViolation

"""
self._check_exception_type(node)
self._check_bare_raise(node)
self.generic_visit(node)

def _check_exception_type(self, node: ast.Raise) -> None:
Expand All @@ -71,6 +74,11 @@ def _check_exception_type(self, node: ast.Raise) -> None:
BaseExceptionRaiseViolation(node, text=exception_name),
)

def _check_bare_raise(self, node: ast.Raise) -> None:
parent = walk.get_closest_parent(node, ast.ExceptHandler)
if not parent and node.exc is None:
self.add_violation(BareRaiseViolation(node))
skarzi marked this conversation as resolved.
Show resolved Hide resolved


@final
@alias('visit_any_function', (
Expand Down