-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Formatter and parser refactoring #7569
Conversation
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
Ok(formatted.print()?) | ||
} | ||
|
||
pub fn format_node<'a>( | ||
root: &'a Mod, | ||
pub fn format_module_ast<'a>( |
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.
The idea here was that you could pass any node eventually (e.g. a single statement)
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.
That makes sense for later, but right now e.g. the comments assume that you have the entire AST present anyway
PR Check ResultsEcosystem✅ ecosystem check detected no changes. |
I got confused and refactored a bit
1aabf59
to
dd8b124
Compare
CodSpeed Performance ReportMerging #7569 will improve performances by 8.46%Comparing Summary
Benchmarks breakdown
|
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.
Would you mind summarizing your changes? It would make reviewing this PR easier.
updated |
I got confused and refactored a bit, now the naming should be more consistent. This is the basis for the range formatting work.
Chages:
format_module
->format_module_source
(format a string)format_node
->format_module_ast
(format a program parsed into an AST)parse_ok_tokens
that takesToken
instead ofResult<Token>
source
consistentlytokens_and_ranges
helperpython_ast
->module
(because that's the type)