basic formatting for ExprDict#5167
Conversation
..._python_formatter/src/snapshots/ruff_python_formatter__tests__black_test__expression_py.snap
Outdated
Show resolved
Hide resolved
99b149a to
cc7df97
Compare
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinuxWindows |
| @@ -0,0 +1,26 @@ | |||
| # before | |||
crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/dict.py
Show resolved
Hide resolved
99f3ed6 to
afd88a6
Compare
| Some(preceding) => preceding.end(), | ||
| None => comment.enclosing_node().start(), | ||
| }; | ||
| let mut tokens_before = SimpleTokenizer::new( |
There was a problem hiding this comment.
first_non_trivia_token_rev might be easier
There was a problem hiding this comment.
The forward version is prefered over rev (when possible) because rev always needs to find the start of the line to assert that the token isn't part of a comment.
# the slash is not a continuation token /
# The following slash is a continuation token
a + /
| b # trailing | ||
| } | ||
|
|
||
| { |
There was a problem hiding this comment.
found a different issue i'm not sure how to approach:
{
** # between
b,
}
{
# before
** # between
b,
}
formats as
{
**b, # between
}
{
**# before
b, # between
}
the # before is causes the stars to be (or stay?) split off again
There was a problem hiding this comment.
Could you try manually calling leading_comments to format bs leading comments before the **?
There was a problem hiding this comment.
seems to work. though i'm slightly confused about how that works. does leading_comments() (in addition to outputting the comment) also remove it from where it would otherwise automatically be added?
is there somewhere i can read about how the formatting system works?
There was a problem hiding this comment.
Yes, that's the case. Formatting comments marks them as formatted and they are then filtered out by the next leading_node_comments call.
There's no explicit documentation for it, but you can read the source. Formatting comments is also based on implementing Format, the same as implementing formatting for any node
7be8e80 to
051b6c0
Compare
| Some(preceding) => preceding.end(), | ||
| None => comment.enclosing_node().start(), | ||
| }; | ||
| let range_start = preceding_end + TextSize::new(1); |
There was a problem hiding this comment.
Using +1 may not be sufficient to skip the comma in case there's whitespace between the preceding node and the comma (or even a comment). I recommend to either call next on the iterator before the loop (with a debug assertion that it matches a comma or {), or matching on the kind in the loop and skipping over commas and {
There was a problem hiding this comment.
nice catch. found an example like that which breaks this code. can fix by skipping first token instead of a single char 👍
how come you suggest asserting the skipped token kind? (it can also be a colon which makes the list a bit tedious to check)
There was a problem hiding this comment.
My main motivation to add asserts if the list of possible kinds is limited (only a few kinds) is to catch incorrect ranges or wrong assumptions early. For example, I messed up the range in #5176. We wouldn't have know about this without the debug assertion being present. The debug assertions further act as documentation. They communicate to readers what kind of tokens are expected.
You can use matches!(token, Some(Token { kind: TokenKind::Colon | TokenKind::Comma | ... })) which, hopefully, makes it less awkward.
group key/value pairs when formatting dict to prefer breaking lines between entries instead of inside them
handle comments between the `**` and the variable name when unpacking dicts inside dict literals can possibly be extended to function arguments, see inline comment
4d5fe36 to
766607e
Compare
766607e to
8d97593
Compare
|
This is awesome. Thank you so much. |
|
Thank you for the mentoring and for your patience with all my questions! |
Summary
basic formatting for ExprDict
Test Plan
snapshots