-
Notifications
You must be signed in to change notification settings - Fork 387
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
Allow docstring in function definition #126
Conversation
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.
It looks basically good!
name="f", | ||
args=ast.arguments( | ||
args=[ast.arg(arg="x")], | ||
kwonlyargs=[], |
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.
Thoughts on removing these empty fields from the tests? Would make it easier to read imo / remove some bloat
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.
How about using ast.parse
as in some other tests?
tree = ast.parse(textwrap.dedent("""\
def f(x):
'''docstring'''
return x
""")).body[0]
assert isinstance(tree, ast.FunctionDef)
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.
def test() -> None:
tree = ast.parse(
"""
def f(x):
'''docstring'''
return x"""
)
I've gone with this format. Unfortunately due to the nature of multiline strings it seems like the def
can't have any preceding indentation, which makes it a bit awkward to read, but still better than before.
There can't be any space between the last non-whitespace character and the closing """
.
I decided to opt against the starting \
because it doesn't actually add anything in terms of the test, and I felt that it's better to keep it simple for readability.
edit: could probably add this function into test_utils
if it's commonly used, but since it's only two cases that can probably get held off
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.
could probably add this function into test_utils if it's commonly used
I think this is not recommended according to the DAMP principle. It is somewhat desirable to write self-contained tests:
https://abseil.io/resources/swe-book/html/ch12.html#tests_and_code_sharing_dampcomma_not_dr
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.
Great, thanks!
Overview
Allows a docstring to be defined at the top of the function.
Details
Docstrings (multiline strings) are represented as an
ast.Expr(ast.Constant)
.When validating that all child statements (minus the last one) in a function definition are of the class
ast.Assign
, we make an exception if the first one is anast.Expr(ast.Constant)
.References
#104