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

feat: introduce DOC503 for checking specific raised exceptions #161

Merged
merged 6 commits into from
Sep 3, 2024

Conversation

Amar1729
Copy link
Contributor

@Amar1729 Amar1729 commented Jul 28, 2024

Closes #158

Compare any raise statements in function bodies to docstring Raises sections and ensures they match. Exceptions specified in a docstring are kept as a sorted list, so duplicate lines will throw DOC503 as well.

Had to introduce a new walker, walk.walk_dfs, but I did add tests for it.

This feature does NOT do any resolution of errors, so something like the following:

try:
    1 / 0
except:
    raise

will not identify any specific exceptions thrown. Similarly, except Exception: raise will expect Exception to be included in the docstring. I think both of those cases are already caught as style violations by certain ruff rules (probably under BLE or TRY?), so it might not be much of a problem.


While this checks names of exceptions it doesn't check order(?) like your issue title mentions, but i'm not entirely sure what was meant by that.

Completely open to feedback / changes, let me know if there's anything that could be cleaned up. In particular, there's one bit I don't love: in visitor.py, i had to do this to parse a common convention when using rest-style docstrings:

elif doc.style == 'sphinx' and raises.description:
    # :raises: Exception: -> 'Exception'
    splitDesc = raises.description.split(':')
    if len(splitDesc) > 1 and ' ' not in splitDesc[0].strip():
        exc = splitDesc[0].strip()
        docRaises.append(exc)

This bit parses docstring lines like :raises: Exception:, which seem to be somewhat common in sphinx docstrings but I don't think are technically allowed by the spec[1][2]? docstring_parser doesn't currently parse those words into type_name. Not sure if this bit of code should be moved into docstring_parser_fork, moved into its own function (I wasn't really sure where else to put it), or if you'd prefer to just not support this line format.

@Amar1729 Amar1729 force-pushed the 158-feat-check-raised-exceptions branch from 3c223e5 to ab605d5 Compare July 28, 2024 08:24
@Amar1729
Copy link
Contributor Author

*couple leftover comments, fixed + squashed.

@Amar1729 Amar1729 force-pushed the 158-feat-check-raised-exceptions branch from ab605d5 to 671dd96 Compare July 28, 2024 08:30
@Amar1729
Copy link
Contributor Author

Amar1729 commented Jul 28, 2024

Interestingly, if you have tox installed with python3.8-.11 vs python3.12, flake8-misc will give slightly different results complaining about ISC001 in visitor_helper (implicitly-concatenated string on older versions, but not 3.12). fixed (and squashed, again).

*aaaaaaand, cercis complains about the splits. joining them each into one line each.

This commit compares any raise statements in function bodies to
docstring Raises sections and ensures they match. Exceptions specified
in a docstring are kept as a sorted list, so duplicate lines will throw
DOC503 as well.
@Amar1729 Amar1729 force-pushed the 158-feat-check-raised-exceptions branch from 671dd96 to 3d62c7d Compare July 28, 2024 08:34
@Amar1729 Amar1729 changed the title feat: introduce DOC503 for checking specific raised exceptions Draft: feat: introduce DOC503 for checking specific raised exceptions Jul 28, 2024
@Amar1729
Copy link
Contributor Author

Amar1729 commented Jul 28, 2024

ah darn, did the visitor walking a bit naively and am not quite getting the right value (OSError) from a testcase like below. will update. fixed, updating main description.

    try:
        pass
    except OSError as e:
        if e.args[0] == 2 and e.filename:
            fp = None
        else:
            raise

@Amar1729 Amar1729 changed the title Draft: feat: introduce DOC503 for checking specific raised exceptions feat: introduce DOC503 for checking specific raised exceptions Jul 28, 2024
Introduces a `walk.walk_dfs` for DFS traversal of a node. This allows us
to more easily keep track of direct parents of nested children, so that
we can find an ExceptHandler that may be containing any Raise statement.

Update tests for walk_dfs and some more complex edge cases for
try/except blocks.
@Amar1729 Amar1729 force-pushed the 158-feat-check-raised-exceptions branch from 1c1d395 to a22d8a1 Compare July 28, 2024 18:51
@jsh9
Copy link
Owner

jsh9 commented Aug 5, 2024

Thank you for helping implement this feature! I'm taking a look at it, and it should take me a day or two.

@@ -33,9 +33,9 @@ def walk(node):


def walk_dfs(node):
"""Depth-first traversal of AST. Modified from walk.walk, above."""
"""Depth-first traversal of AST. Modified from `walk()` in this file"""
Copy link
Owner

Choose a reason for hiding this comment

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

Note: I changed the wording "above" because there's no guarantee that the relative positions of these 2 functions won't be changed in the future.

@@ -376,7 +376,7 @@ def func9(d):
raise

def func10():
# no variable resolution is done. this function looks like it throws "GError".
# no variable resolution is done. this function looks like it throws GError.
Copy link
Owner

Choose a reason for hiding this comment

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

Note: I removed quotation marks just to fit the comment within 80 chars

@jsh9
Copy link
Owner

jsh9 commented Sep 3, 2024

Hi @Amar1729 , sorry for taking so long to get to this. I've been busy with other things last month. Everything looks good now, so I'll merge and publish.

@jsh9 jsh9 merged commit f758604 into jsh9:main Sep 3, 2024
16 checks passed
@Amar1729 Amar1729 deleted the 158-feat-check-raised-exceptions branch September 3, 2024 04:42
@Amar1729
Copy link
Contributor Author

Amar1729 commented Sep 3, 2024

@jsh9 no worries, I get it. thanks for coming back around to it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a feature to check exceptions (name and order)
2 participants