-
Notifications
You must be signed in to change notification settings - Fork 5
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
PythonPrinter
in Python
#128
Conversation
from .. import PrintOutputCapture, Cursor, Markers, Tree, Marker | ||
from ..visitor import T | ||
|
||
JAVA_MARKER_WRAPPER: Callable[[str], str] = lambda out: f"/*~~{out}{'~~' if out else ''}>*/""" |
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.
Python unfortunately doesn't have multi-line comments, only end-of-line comments. This means that adding markers will actually break the code. But since code with markup typically isn't compiled anyway, this is not much of an issue. I think I would just leave it like this.
JAVA_MARKER_WRAPPER | ||
)) | ||
|
||
def after_syntax( |
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.
I suggest we split this into after_tree
and after_markers
methods. That way we don't require ugly hacks.
rewrite/rewrite/test.py
Outdated
def print_source_file(source_file: SourceFile) -> str: | ||
c = PrintOutputCapture(0) | ||
PythonPrinter().visit(source_file, c) | ||
return c.get_out() |
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.
If we merge main then this function should no longer be required.
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.
removed
rewrite/rewrite/test.py
Outdated
before_printed = source_file.print_all() if USE_REMOTE else print_source_file(source_file) | ||
assert before_printed == source_spec.before |
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.
By doing what I suggest above we should be able to retain the original assert source_file.print_all() == source_spec.before
here. The if USE_REMOTE
guard is still required around the two other statements.
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.
Fixed
rewrite/rewrite/test.py
Outdated
@@ -127,7 +138,7 @@ def rewrite_run(*source_specs: Iterable[SourceSpec], spec: Optional[RecipeSpec] | |||
if res._before and res._after: | |||
source_spec = spec_by_source_file[res._before] | |||
source_spec.after_recipe(res._after) | |||
after_printed = res._after.print_all() | |||
after_printed = res._after.print_all() if USE_REMOTE else print_source_file(res._after) |
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.
Same here.
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.
Fixed
What's changed?
This PR adds an initial version of the
PythonPrinter
written in Python.What's your motivation?
Currently, there is an implementation available in Java; however, it requires starting a Java remoting server. The Python implementation improves ease of use, speed, and debugging ability.
Anything in particular you'd like reviewers to focus on?
The current implementation mimics the Java version, where the printer consists of two classes, the
PythonPrinter
, and thePythonJavaPrinter
, which inherits fromJavaPrinter
to repurpose some logic of theJavaPrinter
. As this is not yet implemented in Python, we might consider simply contracting them into one class.Anyone you would like to review specifically?
@knutwannheden
Have you considered any alternatives or workarounds?
Any additional context
Checklist