Fix AST handling of docstrings and __future__ ordering #1673
Conversation
hy/compiler.py
Outdated
| body += [result.stmts.pop(0)] | ||
|
|
||
| body += sorted(compiler.imports_as_stmts(tree) + result.stmts, | ||
| key=import_order_key) |
There was a problem hiding this comment.
Couldn't you just write something like key=lambda x: not (isinstance(x, ast.ImportFrom) and x.module == '__future__')?
There was a problem hiding this comment.
Yeah, probably. This started from an elaborate scheme to check ast.ImportFrom and multi-import ast.Imports, then I realized only from __future__ import ... statements needed to be handled and cut out parts from there.
tests/compilers/test_ast.py
Outdated
| """Confirm that docstrings come before import statements in AST output.""" | ||
| hy_tree = import_buffer_to_hst('"hello world"\n(print (first [1 2]))') | ||
| hy_ast = hy_compile(hy_tree, 'test') | ||
| assert hy_ast.body[0].value.s == 'hello world' |
There was a problem hiding this comment.
I think it would be a better test to add a module docstring to resources/pydemo.hy and check for it in test_hy2py.
There was a problem hiding this comment.
You fine leaving that test in there, though? I like its "directness".
There was a problem hiding this comment.
It uses an internal function (import_buffer_to_hst), and it relies on the structure of Python AST, to test for a bug that's readily visible in user space. So I'd say we should get rid of it if we don't need it.
There was a problem hiding this comment.
Looks like the tests in test_ast.py are using can_compile instead; I'll make the other tests here use that as well.
Otherwise, I can replace this one with an assertion to test_hy2py that checks m.__doc__.
brandonwillard
force-pushed
the
fix-docstring-with-imports
branch
4 times, most recently
from
68ffb3c
to
7f52f68
Compare
This closes hylang#1367 and closes hylang#1540
Kodiologist
force-pushed
the
fix-docstring-with-imports
branch
from
b8790ce
to
f4e5f02
Compare
This closes #1367, closes #1540