Skip to content

Simplify formatting of strings by using flags from the AST nodes #10489

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

Merged
merged 4 commits into from
Mar 20, 2024

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Mar 20, 2024

Summary

This PR simplifies the formatter by using the flags available on AST nodes, which were added in #10298 and refined in #10314. These can be used to query the quoting style and prefix used by a string. (Currently, we compute these by reparsing them from the source code.)

I was hoping this might lead to a speedup -- locally for me, it seems to be performance-neutral. It's (in my opinion) a nice simplification, anyway.

Each commit is atomic; the full test suite passes on each commit.

Test Plan

cargo test

@AlexWaygood AlexWaygood added internal An internal refactor or improvement formatter Related to the formatter labels Mar 20, 2024
Copy link
Contributor

github-actions bot commented Mar 20, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@AlexWaygood AlexWaygood force-pushed the formatter-string-flags-2 branch from c51c6d5 to d10c4b8 Compare March 20, 2024 12:48
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Nice. It allows us to delete quiet a lot of code.

There are two concerns:

  • Using impl StringPart leads to monomorphization of not entirely trivial functions -> More generated code. I recommend extracting a struct that holds just the information we need to avoid this
  • We should move StringKind out of the parser crate because I want to make ruff_python_formatter independent of the parser crate eventually. It should only depend on the ruff_python_ast and not be concerned about how the source file was parsed (that will happen in a caller crate).

@AlexWaygood AlexWaygood force-pushed the formatter-string-flags-2 branch from d10c4b8 to f0a47a7 Compare March 20, 2024 14:54
@@ -2108,6 +2106,439 @@ impl From<BytesLiteral> for Expr {
}
}

bitflags! {
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if all this belongs in nodes.rs. ruff_python_ast::str.rs didn't feel right, though, since nodes.rs depends on str.rs (it would have created a circular dependency between the two files.

@AlexWaygood
Copy link
Member Author

Thanks! I added a commit to move the relevant abstractions from ruff_python_parser to ruff_python_ast, and addressed your other comments

@AlexWaygood AlexWaygood requested a review from MichaReiser March 20, 2024 15:26
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Thank you

@AlexWaygood AlexWaygood merged commit 7caf0d0 into astral-sh:main Mar 20, 2024
@AlexWaygood AlexWaygood deleted the formatter-string-flags-2 branch March 20, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter internal An internal refactor or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants