Format ExprIfExp (ternary operator) by konstin · Pull Request #5597 · astral-sh/ruff

konstin · 2023-07-07T16:39:58Z

Summary

Format ExprIfExp, also known as the ternary operator or inline if. It can look like

a1 = 1 if True else 2

but also

b1 = (
    # We return "a" ...
    "a" # that's our True value
    # ... if this condition matches ...
    if True # that's our test
    # ... otherwise we return "b§
    else "b" # that's our False value
)

This also fixes a visitor order bug.

The jaccard index on django goes from 0.911 to 0.915.

Test Plan

I added fixtures without and with comments in strange places.

github-actions · 2023-07-07T16:56:39Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00     10.4±0.16ms     3.9 MB/sec    1.01     10.5±0.25ms     3.9 MB/sec
formatter/numpy/ctypeslib.py               1.01      2.3±0.05ms     7.3 MB/sec    1.00      2.2±0.07ms     7.4 MB/sec
formatter/numpy/globals.py                 1.00    262.3±8.84µs    11.3 MB/sec    1.03   269.1±34.82µs    11.0 MB/sec
formatter/pydantic/types.py                1.00      5.0±0.12ms     5.1 MB/sec    1.00      5.0±0.14ms     5.1 MB/sec
linter/all-rules/large/dataset.py          1.00     18.4±0.62ms     2.2 MB/sec    1.03     18.9±0.59ms     2.1 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.4±0.13ms     3.8 MB/sec    1.02      4.5±0.13ms     3.7 MB/sec
linter/all-rules/numpy/globals.py          1.00   561.2±18.73µs     5.3 MB/sec    1.04   583.8±19.25µs     5.1 MB/sec
linter/all-rules/pydantic/types.py         1.00      8.0±0.25ms     3.2 MB/sec    1.01      8.1±0.15ms     3.1 MB/sec
linter/default-rules/large/dataset.py      1.00      9.0±0.24ms     4.5 MB/sec    1.02      9.2±0.23ms     4.4 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01  1976.8±49.64µs     8.4 MB/sec    1.00  1950.9±37.93µs     8.5 MB/sec
linter/default-rules/numpy/globals.py      1.03   236.6±14.34µs    12.5 MB/sec    1.00   230.3±10.27µs    12.8 MB/sec
linter/default-rules/pydantic/types.py     1.01      4.1±0.10ms     6.2 MB/sec    1.00      4.1±0.11ms     6.3 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.09     12.7±0.47ms     3.2 MB/sec    1.00     11.7±0.52ms     3.5 MB/sec
formatter/numpy/ctypeslib.py               1.00      2.6±0.13ms     6.4 MB/sec    1.01      2.6±0.14ms     6.3 MB/sec
formatter/numpy/globals.py                 1.00   293.4±17.23µs    10.1 MB/sec    1.06   311.0±15.72µs     9.5 MB/sec
formatter/pydantic/types.py                1.00      5.9±0.27ms     4.3 MB/sec    1.00      5.9±0.21ms     4.3 MB/sec
linter/all-rules/large/dataset.py          1.00     22.1±0.85ms  1881.2 KB/sec    1.00     22.1±0.88ms  1886.4 KB/sec
linter/all-rules/numpy/ctypeslib.py        1.03      5.7±0.18ms     2.9 MB/sec    1.00      5.6±0.23ms     3.0 MB/sec
linter/all-rules/numpy/globals.py          1.00   677.6±40.28µs     4.4 MB/sec    1.01   682.4±49.26µs     4.3 MB/sec
linter/all-rules/pydantic/types.py         1.02      9.6±0.48ms     2.6 MB/sec    1.00      9.5±0.44ms     2.7 MB/sec
linter/default-rules/large/dataset.py      1.00     10.9±0.43ms     3.7 MB/sec    1.02     11.1±0.37ms     3.7 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.10      2.3±0.08ms     7.1 MB/sec    1.00      2.1±0.10ms     7.9 MB/sec
linter/default-rules/numpy/globals.py      1.02   286.3±15.43µs    10.3 MB/sec    1.00   280.7±30.19µs    10.5 MB/sec
linter/default-rules/pydantic/types.py     1.05      4.9±0.18ms     5.2 MB/sec    1.00      4.7±0.23ms     5.4 MB/sec

MichaReiser

The jaccard index on django afterwards is 0.915.

What was the value before?

I think it could make sense to give it a quick try to add the if and else tokens to the SimpleTokenizer.

crates/ruff_python_ast/src/visitor/preorder.rs

crates/ruff_python_formatter/src/comments/placement.rs

MichaReiser · 2023-07-07T17:06:01Z

crates/ruff_python_formatter/src/comments/placement.rs

+/// Looks for a multi char token in the range that contains no other tokens. `SimpleTokenizer` only
+/// works with single char tokens so we check that we have the right token by string comparison.


It should be possible to add support for if and else. Comments are already non-single token strings.
The backwards lexing is a bit more annoying but should be doable as well.

i didn't want to change the single-char-ness of the tokenizer just for this PR, but if you want i can do the refactoring in another PR

MichaReiser · 2023-07-07T17:07:09Z

crates/ruff_python_formatter/src/comments/placement.rs

+
+    if if_token.range.start() < comment.slice().start()
+        && comment.slice().start() < test.start()
+        && comment.line_position().is_end_of_line()


I recommend moving the line_position check to the very top to avoid unnecessarily lexing own line comments.

The order for ternary operators is different than that for if statements

konstin · 2023-07-07T19:02:56Z

What was the value before?

0.911, added it to the description

konstin · 2023-07-07T19:06:25Z

I think it could make sense to give it a quick try to add the if and else tokens to the SimpleTokenizer.

I add it to my TODO list

davidszotten · 2023-07-07T19:08:49Z

I think it could make sense to give it a quick try to add the if and else tokens to the SimpleTokenizer.

I add it to my TODO list

ooh, maybe in and if could be useful for for #5600

i hadn't looked in detail but didn't consider it since it only has "special" chars so far, no words

davidszotten · 2023-07-08T09:45:45Z

I think it could make sense to give it a quick try to add the if and else tokens to the SimpleTokenizer.

I add it to my TODO list

i took a stab at it: #5610

konstin force-pushed the format_expr_if_exp branch from d320220 to 519076c Compare July 7, 2023 16:41

MichaReiser approved these changes Jul 7, 2023

View reviewed changes

konstin added 3 commits July 7, 2023 21:00

Fix preorder visitor

843a150

The order for ternary operators is different than that for if statements

Format ExprIfExp (ternary operator)

32fbc1e

Review

730ebd7

konstin force-pushed the format_expr_if_exp branch from 1b825b5 to 730ebd7 Compare July 7, 2023 19:04

konstin enabled auto-merge (squash) July 7, 2023 19:06

konstin mentioned this pull request Jul 7, 2023

📋 Formatter: Syntax Formatting #4798

Closed

72 tasks

konstin merged commit 0b9af03 into main Jul 7, 2023

konstin deleted the format_expr_if_exp branch July 7, 2023 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Format ExprIfExp (ternary operator)#5597

Format ExprIfExp (ternary operator)#5597
konstin merged 3 commits intomainfrom
format_expr_if_exp

konstin commented Jul 7, 2023 •

edited

Loading

Uh oh!

github-actions bot commented Jul 7, 2023 •

edited

Loading

Uh oh!

MichaReiser left a comment

Uh oh!

Uh oh!

Uh oh!

MichaReiser Jul 7, 2023

Uh oh!

konstin Jul 7, 2023

Uh oh!

MichaReiser Jul 7, 2023

Uh oh!

konstin Jul 7, 2023

Uh oh!

konstin commented Jul 7, 2023

Uh oh!

konstin commented Jul 7, 2023

Uh oh!

davidszotten commented Jul 7, 2023

Uh oh!

davidszotten commented Jul 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		/// Looks for a multi char token in the range that contains no other tokens. `SimpleTokenizer` only
		/// works with single char tokens so we check that we have the right token by string comparison.

Conversation

konstin commented Jul 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

github-actions bot commented Jul 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Check Results

Ecosystem

Benchmark

Linux

Windows

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MichaReiser Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

konstin Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

MichaReiser Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

konstin Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

konstin commented Jul 7, 2023

Uh oh!

konstin commented Jul 7, 2023

Uh oh!

davidszotten commented Jul 7, 2023

Uh oh!

davidszotten commented Jul 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

konstin commented Jul 7, 2023 •

edited

Loading

github-actions bot commented Jul 7, 2023 •

edited

Loading