basic formatting for ExprDict #5167

davidszotten · 2023-06-17T22:08:38Z

Summary

basic formatting for ExprDict

Test Plan

snapshots

..._python_formatter/src/snapshots/ruff_python_formatter__tests__black_test__expression_py.snap

github-actions · 2023-06-17T22:20:25Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.01      6.8±0.01ms     5.9 MB/sec    1.00      6.8±0.01ms     6.0 MB/sec
formatter/numpy/ctypeslib.py               1.01  1371.5±10.82µs    12.1 MB/sec    1.00   1357.7±4.97µs    12.3 MB/sec
formatter/numpy/globals.py                 1.01    132.6±0.28µs    22.3 MB/sec    1.00    131.8±0.17µs    22.4 MB/sec
formatter/pydantic/types.py                1.01      2.8±0.01ms     9.2 MB/sec    1.00      2.7±0.01ms     9.3 MB/sec
linter/all-rules/large/dataset.py          1.01     13.7±0.03ms     3.0 MB/sec    1.00     13.6±0.03ms     3.0 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      3.5±0.00ms     4.8 MB/sec    1.00      3.4±0.00ms     4.9 MB/sec
linter/all-rules/numpy/globals.py          1.01    363.3±9.01µs     8.1 MB/sec    1.00    359.6±0.99µs     8.2 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.0±0.01ms     4.2 MB/sec    1.00      6.0±0.01ms     4.2 MB/sec
linter/default-rules/large/dataset.py      1.00      7.0±0.01ms     5.8 MB/sec    1.01      7.0±0.01ms     5.8 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00   1466.7±3.35µs    11.4 MB/sec    1.00   1469.6±3.14µs    11.3 MB/sec
linter/default-rules/numpy/globals.py      1.00    157.4±0.20µs    18.7 MB/sec    1.00    157.8±0.19µs    18.7 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.2±0.00ms     8.0 MB/sec    1.00      3.2±0.01ms     8.0 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      9.9±0.43ms     4.1 MB/sec    1.00      9.9±0.43ms     4.1 MB/sec
formatter/numpy/ctypeslib.py               1.00  1983.3±80.85µs     8.4 MB/sec    1.02      2.0±0.12ms     8.2 MB/sec
formatter/numpy/globals.py                 1.00   205.7±13.57µs    14.3 MB/sec    1.04   214.4±18.15µs    13.8 MB/sec
formatter/pydantic/types.py                1.03      4.1±0.20ms     6.2 MB/sec    1.00      4.0±0.15ms     6.4 MB/sec
linter/all-rules/large/dataset.py          1.00     20.3±0.81ms     2.0 MB/sec    1.00     20.2±0.79ms     2.0 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      5.3±0.25ms     3.2 MB/sec    1.00      5.2±0.21ms     3.2 MB/sec
linter/all-rules/numpy/globals.py          1.00   642.4±27.91µs     4.6 MB/sec    1.00   640.4±34.31µs     4.6 MB/sec
linter/all-rules/pydantic/types.py         1.08      9.3±0.49ms     2.7 MB/sec    1.00      8.7±0.29ms     2.9 MB/sec
linter/default-rules/large/dataset.py      1.00     10.3±0.32ms     4.0 MB/sec    1.00     10.2±0.54ms     4.0 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.04      2.2±0.11ms     7.4 MB/sec    1.00      2.2±0.12ms     7.7 MB/sec
linter/default-rules/numpy/globals.py      1.00   267.5±17.57µs    11.0 MB/sec    1.00   267.8±20.70µs    11.0 MB/sec
linter/default-rules/pydantic/types.py     1.02      4.8±0.22ms     5.4 MB/sec    1.00      4.7±0.21ms     5.5 MB/sec

konstin

This already looks really good for such a complex node!

konstin · 2023-06-18T11:28:40Z

crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/dict.py

@@ -0,0 +1,26 @@
+# before


good test case

crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/dict.py

crates/ruff_python_formatter/src/expression/expr_dict.rs

crates/ruff_python_formatter/src/expression/expr_compare.rs

crates/ruff_python_formatter/src/comments/placement.rs

crates/ruff_python_formatter/src/expression/expr_dict.rs

konstin · 2023-06-19T11:13:17Z

crates/ruff_python_formatter/src/comments/placement.rs

+            Some(preceding) => preceding.end(),
+            None => comment.enclosing_node().start(),
+        };
+        let mut tokens_before = SimpleTokenizer::new(


first_non_trivia_token_rev might be easier

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 + /

davidszotten · 2023-06-19T14:00:06Z

crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/dict.py

+b # trailing
+}
+
+{


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

Could you try manually calling leading_comments to format bs leading comments before the **?

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?

Yes, that's the case. Formatting comments marks them as formatted and they are then filtered out by the next leading_node_comments call.

ruff/crates/ruff_python_formatter/src/comments/format.rs

Line 51 in 48f4f2d

comment.mark_formatted();

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

MichaReiser

Nice!

MichaReiser · 2023-06-19T21:05:57Z

crates/ruff_python_formatter/src/comments/placement.rs

+        Some(preceding) => preceding.end(),
+        None => comment.enclosing_node().start(),
+    };
+    let range_start = preceding_end + TextSize::new(1);


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 {

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)

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

MichaReiser · 2023-06-20T09:18:02Z

This is awesome. Thank you so much.

davidszotten · 2023-06-20T10:02:08Z

Thank you for the mentoring and for your patience with all my questions!

davidszotten commented Jun 17, 2023

View reviewed changes

..._python_formatter/src/snapshots/ruff_python_formatter__tests__black_test__expression_py.snap Outdated Show resolved Hide resolved

davidszotten force-pushed the format-expr-dict branch from 99b149a to cc7df97 Compare June 17, 2023 22:11

konstin approved these changes Jun 18, 2023

View reviewed changes

konstin added the formatter Related to the formatter label Jun 18, 2023

davidszotten mentioned this pull request Jun 19, 2023

formatter: debug panic in placement::find_pos_only_slash_offset #5176

Closed

davidszotten force-pushed the format-expr-dict branch 2 times, most recently from 99f3ed6 to afd88a6 Compare June 19, 2023 08:21

MichaReiser approved these changes Jun 19, 2023

View reviewed changes

konstin reviewed Jun 19, 2023

View reviewed changes

davidszotten commented Jun 19, 2023

View reviewed changes

davidszotten force-pushed the format-expr-dict branch 2 times, most recently from 7be8e80 to 051b6c0 Compare June 19, 2023 19:28

MichaReiser approved these changes Jun 19, 2023

View reviewed changes

MichaReiser linked an issue Jun 20, 2023 that may be closed by this pull request

Formatter: Dict #5206

Closed

davidszotten added 6 commits June 20, 2023 10:05

basic formatting for ExprDict

b4a4f03

fmt ExprDict: group k/v pairs

80ac935

group key/value pairs when formatting dict to prefer breaking lines between entries instead of inside them

formatter: comments inside dict unpacking

503bed9

handle comments between the `**` and the variable name when unpacking dicts inside dict literals can possibly be extended to function arguments, see inline comment

handle leading comments for split dict unpacking

0b2ed7a

improve perf of handle_dict_unpacking_comment

6c68dae

better skipping of the preceding token

fb8fa49

davidszotten force-pushed the format-expr-dict branch from 4d5fe36 to 766607e Compare June 20, 2023 09:10

check the skipped token type

8d97593

davidszotten force-pushed the format-expr-dict branch from 766607e to 8d97593 Compare June 20, 2023 09:12

MichaReiser enabled auto-merge (squash) June 20, 2023 09:18

MichaReiser merged commit 773e79b into astral-sh:main Jun 20, 2023

davidszotten deleted the format-expr-dict branch July 7, 2023 20:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

basic formatting for ExprDict #5167

basic formatting for ExprDict #5167

davidszotten commented Jun 17, 2023

github-actions bot commented Jun 17, 2023 •

edited

Loading

konstin left a comment •

edited

Loading

konstin Jun 18, 2023

konstin Jun 19, 2023

MichaReiser Jun 19, 2023

davidszotten Jun 19, 2023

konstin Jun 19, 2023

davidszotten Jun 19, 2023

MichaReiser Jun 19, 2023

MichaReiser left a comment

MichaReiser Jun 19, 2023

davidszotten Jun 19, 2023

MichaReiser Jun 20, 2023

MichaReiser commented Jun 20, 2023

davidszotten commented Jun 20, 2023

basic formatting for ExprDict #5167

basic formatting for ExprDict #5167

Conversation

davidszotten commented Jun 17, 2023

Summary

Test Plan

github-actions bot commented Jun 17, 2023 • edited Loading

PR Check Results

Ecosystem

Benchmark

Linux

Windows

konstin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser commented Jun 20, 2023

davidszotten commented Jun 20, 2023

github-actions bot commented Jun 17, 2023 •

edited

Loading

konstin left a comment •

edited

Loading