-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fix AST safety check false negative #4270
Conversation
Fixes psf#4268 Previously we would allow whitespace changes in all strings, now only in docstrings.
for more information, see https://pre-commit.ci
Seems like we got unlucky and someone added 3.12+ syntax to a diff-shades project just two hours ago (koaning/scikit-lego@64485a9#diff-db41f509a7991f7b7579a72fe7bdba315b5ab82bdde2f237c6a53d03396c0081R97 line 97). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, two small things
Co-authored-by: Shantanu <[email protected]>
Co-authored-by: Shantanu <[email protected]>
Fixes psf#4268 Previously we would allow whitespace changes in all strings, now only in docstrings. Co-authored-by: Shantanu <[email protected]>
@@ -110,6 +110,10 @@ def lib2to3_unparse(node: Node) -> str: | |||
return code | |||
|
|||
|
|||
class ASTSafetyError(Exception): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably inherit from AssertionError
since you are replacing raise AssertionError
with this new exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't provide compatibility guarantees for this function. I don't think inheriting from AssertionError makes semantic sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the response. That makes sense considering that many people probably don't use this functionality directly and I maybe agree inheriting from AssertionError
is odd from a semantic/purity standpoint, even if it's less practical for compatibility (which as you mention is a non-goal here).
…no longer raises an AssertionError
Fixes #4268
Previously we would allow whitespace changes in all strings, now
only in docstrings.