Module docstrings in 3.7 are not part of Module node anymore #3531
Conversation
testing/test_assertrewrite.py
Outdated
| @@ -70,10 +70,16 @@ class TestAssertionRewrite(object): | |||
| def test_place_initial_imports(self): | |||
| s = """'Doc string'\nother = stuff""" | |||
| m = rewrite(s) | |||
| # Module docstrings in 3.7 are part of Module node, it's not in the body | |||
| # Module docstrings in 3.8 are part of Module node, it's not in the body | |||
There was a problem hiding this comment.
do we know for sure it will change in 3.8, or do we need a tracking issue?
There was a problem hiding this comment.
Not sure, let's ask @ncoghlan
There was a problem hiding this comment.
We're not sure yet what's going to happen in 3.8, as the key point that we didn't take into account this time around is the significant difference that moving module docstrings around creates between doing ast.parse("'Hello'", mode="exec").body[0].value and ast.parse("'Hello'", mode="eval").body.
I suspect what we'll end up going with is Naoki Inada's idea of introducing a dedicated DocString node into the AST, since that still allows a bunch of special case heuristics in the compiler to be tidied up (since they can dispatch directly on node type instead), without losing the location information for the docstring, and without making them disappear from the node tree entirely.
There was a problem hiding this comment.
LGTM, please take a look at my comment and let's see if we need to update the comment as @RonnyPfannschmidt mentioned.
Thanks @hroncok!
testing/test_assertrewrite.py
Outdated
| # We have a complicated sys.version_info if in here to ease testing on | ||
| # various Python 3.7 versions, but we should remove the 3.7 check after | ||
| # 3.7 is released as stable | ||
| if ( |
There was a problem hiding this comment.
Could you move the check to its own function, moving the explanatory comment to its docstring? This will make the code easier to follow.
testing/test_assertrewrite.py
Outdated
| # Module docstrings in some new Python versions are part of Module node | ||
| # It's not in the body so we remove it so the following body items have | ||
| # the same indexes on all Python versions: | ||
| if python_version_has_docstring_in_module_node(): |
There was a problem hiding this comment.
sorry for the nitpick, since in all 3 cases the following code is the same can we please factor it over as well
There was a problem hiding this comment.
Pushed in 8dff229. Feel free to squash or not squash when merging.
testing/test_assertrewrite.py
Outdated
| various Python 3.7 versions, but we should remove the 3.7 check after | ||
| 3.7 is released as stable to make this check more straightforward. | ||
| """ | ||
| if sys.version_info < (3, 8) or (3, 7) <= sys.version_info <= (3, 7, 0, "beta", 4): |
There was a problem hiding this comment.
Looking at this, it might not work.
There was a problem hiding this comment.
And it doesn't. The second part should be inverted.
testing/test_assertrewrite.py
Outdated
| assert isinstance(m.body[0], ast.Expr) | ||
| assert isinstance(m.body[0].value, ast.Str) | ||
| del m.body[0] | ||
| return m |
There was a problem hiding this comment.
since this is a mutating helper, i propose not having a return value
|
NB: I've checked that tests pass with both 3.7.0b4 and 3.7.0b5. |
Fixes #3530