-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Configurable "unparse mode" for ruff_python_codegen::Generator
#21041
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
Configurable "unparse mode" for ruff_python_codegen::Generator
#21041
Conversation
|
| /// Like [`round_trip`] but configure the [`Generator`] with the requested | ||
| /// `indentation`, `line_ending` and `preferred_quote` settings. | ||
| /// `indentation`, `line_ending`, `preferred_quote` and `forced_tuple_parentheses` settings. | ||
| fn round_trip_with( |
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 signature here becomes a bit awkward. Should we introduce a GeneratorOptions struct with:
indentationline_endingpreferred_quote: Optionforced_tuple_parentheses
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.
sounds good
ruff_python_codegen::Generatorruff_python_codegen::Generator
| pub enum UnparseMode { | ||
| #[default] | ||
| Default, | ||
| Ast, | ||
| } |
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.
Nit: I think just Mode is enough. I'd rename Ast to AstUnparse and document that the intent is to emit the same output as Python's ast.unparse method
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.
np, would you prefer this documented at the enum variant, or the Generator::with_unparse_mode?
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 documenting it at the variant level
| let quote_style = match self.unparse_mode { | ||
| UnparseMode::Default => flags.quote_style(), | ||
| UnparseMode::Ast => Quote::Single, | ||
| }; |
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.
We could make this a method on Mode insteada of repeating it at every call site
impl Mode {
const fn quote_style(self, flags: AnyStringFlags) -> QuoteStyle {
...
}
}
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 guess that it would make sense to move the tuple precedence value to it's own method as well, right? although it's only used once it kind of makes more sense to me for some reason. I'll do that if you think it's a good idea
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'm less concerned about the tuple case because it's used exactly once.
| /// - [`Default`](`Mode::Default`): Output of `[AnyStringFlags.quote_style`]. | ||
| /// - [`AstUnparse`](`Mode::AstUnparse`): Always return [`Quote::Single`]. | ||
| #[must_use] | ||
| fn quote_style(&self, flags: impl StringFlags) -> Quote { |
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.
had to use generics because this gets called with either AnyStringFlags or BytesLiteralFlags
| use crate::stylist::Indentation; | ||
|
|
||
| use super::Generator; | ||
| use super::{Generator, Mode as UnparseMode}; |
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.
Renamed to UnparsedMode here because the name collides with
use ruff_python_parser::{..., Mode, ...};|
tysm @MichaReiser <3 ! |
Summary
At RustPython we want to fully replace https://github.com/RustPython/RustPython/blob/153d0eef51ea109d8a322fd351678847b2fb8fe2/compiler/codegen/src/unparse.rs.
Currently our only regression when using Ruff's unparsing, is forcing tuple parentheses.
Test Plan
Added tests