Skip to content

Rewrite unparse to use ast.NodeVisitor#7497

Merged
tk0miya merged 2 commits intosphinx-doc:3.xfrom
eric-wieser:tidy-ast-unparse
May 7, 2020
Merged

Rewrite unparse to use ast.NodeVisitor#7497
tk0miya merged 2 commits intosphinx-doc:3.xfrom
eric-wieser:tidy-ast-unparse

Conversation

@eric-wieser
Copy link
Copy Markdown
Contributor

@eric-wieser eric-wieser commented Apr 17, 2020

This should make it possible to reuse the same visitor to generate RST code.

Follows on from #7494

This is just a refactoring, motivated by:

def visit_List(self, node):
return "[" + ", ".join(self.visit(e) for e in node.elts) + "]"

def unparse_arguments(node: ast.arguments) -> str:
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Diff here is mostly just an extra level of indent, I'd recommend checking the "ignore whitespace" box to review.

This should make it possible to reuse the same visitor to generate RST code.
This will make it easier to remove them all at once in future
@eric-wieser eric-wieser marked this pull request as ready for review April 17, 2020 15:58
@eric-wieser
Copy link
Copy Markdown
Contributor Author

Had a bit of a fight with mypy, but CI now passes.

Copy link
Copy Markdown
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

LGTM with nits.

else:
raise NotImplementedError('Unable to parse %s object' % type(node).__name__)

if sys.version_info < (3, 8):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO, it would be better to move such online-patch codes to the bottom of the class definition. In addition, it would be better to sort methods alphabetically.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was trying to keep them in the same order as before. Happy to alphabetize though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

done in #7625

@tk0miya
Copy link
Copy Markdown
Member

tk0miya commented Apr 27, 2020

Remind: I will merge this after 1) conflicts resolved and 2) methods are sorted.

Not in a hurry. Please update if you have enough time.

@eric-wieser
Copy link
Copy Markdown
Contributor Author

Thanks for taking over.

I'll see if I can rebase my follow-up work.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants