Format `raise` statement by LouisDISPA · Pull Request #5595 · astral-sh/ruff

LouisDISPA · 2023-07-07T16:01:52Z

Summary

This PR implements the formatting of raise statements. I haven't looked at the black implementation, this is inspired from from the return statements formatting.

Test Plan

The black differences with insta.

I also compared manually some edge cases with very long string and call chaining and it seems to do the same formatting as black.

There is one issue:

# input

raise OsError(
    "aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa"
) from a.aaaaa(aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa).a(aaaa)


# black

raise OsError(
    "aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa"
) from a.aaaaa(
    aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa
).a(
    aaaa
)


# ruff

raise OsError(
    "aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa"
) from a.aaaaa(
    aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa
).a(aaaa)

But I'm not sure this diff is the raise formatting implementation.

MichaReiser

Yeah, you can ignore that issue. This is related to expression formatting.

It would be great if you could add a few more tests with comments in different positions to see that this doesn't cause formatting errors:

raise (
	# comment
	a
) from (
	b
	# comment
)

MichaReiser · 2023-07-07T16:31:14Z

crates/ruff_python_formatter/src/statement/stmt_raise.rs

+        if let Some(value) = exc {
+            write!(
+                f,
+                [space(), value.format().with_options(Parenthesize::Optional)]


I think we'll need Parenthesize::IfBreaks here to support

raise aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbb + cccccccccccccccccccccc + ddddddddddddddddddddddddd

(Yes, this is a very made up example)

I was using Parenthesize::IfBreaks at first but then I noticed that with black:

# input raise (aaaaa.aaa(a).a) from (aksjdhflsakhdflkjsadlfajkslhf) raise bbbbb.bbb(bb) from c # black output raise (aaaaa.aaa(a).a) from (aksjdhflsakhdflkjsadlfajkslhf) raise bbbbb.bbb(bb) from c # ruff (IfBreak) ouput raise aaaaa.aaa(a).a from aksjdhflsakhdflkjsadlfajkslhf raise bbbbb.bbb(bb) from c

And I found that Parenthesize::Optional should work if I understand the doc correctly. And I got:

# ruff (Optional) output raise (aaaaa.aaa(a).a) from (aksjdhflsakhdflkjsadlfajkslhf) raise bbbbb.bbb(bb) from c

But with your exemple I found another issue:
Black does not break if there is no parentheses but ruff does.

# input raise aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbb + cccccccccccccccccccccc + ddddddddddddddddddddddddd raise (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbb + cccccccccccccccccccccc + ddddddddddddddddddddddddd) # black output raise aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbb + cccccccccccccccccccccc + ddddddddddddddddddddddddd raise ( aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbb + cccccccccccccccccccccc + ddddddddddddddddddddddddd ) # ruff (Optional) ouput raise ( aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbb + cccccccccccccccccccccc + ddddddddddddddddddddddddd ) raise ( aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbb + cccccccccccccccccccccc + ddddddddddddddddddddddddd )

And Parenthesize::Preserve does not work.
Is there something like: if in parenthesis then format else don't.

I find it weird that black does not format these in the way that ruff does in the examples you shared 🤔

I found the same behaviour with the lambda body:

# input lambda a: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbb + cccccccccccccccccccccc + ddddddddddddddddddddddddd lambda a: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbb + (cccccccccccccccccccccc + ddddddddddddddddddddddddd) lambda a: (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbb + cccccccccccccccccccccc + ddddddddddddddddddddddddd) # black output lambda a: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbb + cccccccccccccccccccccc + ddddddddddddddddddddddddd lambda a: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbb + ( cccccccccccccccccccccc + ddddddddddddddddddddddddd ) lambda a: ( aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbb + cccccccccccccccccccccc + ddddddddddddddddddddddddd )

and the yield value:

# input yield aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbb + cccccccccccccccccccccc + ddddddddddddddddddddddddd yield aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbb + (cccccccccccccccccccccc + ddddddddddddddddddddddddd) yield (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbb + cccccccccccccccccccccc + ddddddddddddddddddddddddd) # black output yield aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbb + cccccccccccccccccccccc + ddddddddddddddddddddddddd yield aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbb + ( cccccccccccccccccccccc + ddddddddddddddddddddddddd ) yield ( aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbb + cccccccccccccccccccccc + ddddddddddddddddddddddddd )

Sorry, I confused Parentheses::Optional and Parentheses::Preserve. Optional should be fine here. I find black's behavior surprising. Either way, this is something you can ignore for your PR, because it affects all expression formatting.

crates/ruff_python_formatter/src/statement/stmt_raise.rs

github-actions · 2023-07-07T16:37:32Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      8.4±0.04ms     4.8 MB/sec    1.01      8.5±0.08ms     4.8 MB/sec
formatter/numpy/ctypeslib.py               1.00   1797.8±3.34µs     9.3 MB/sec    1.03   1852.1±3.65µs     9.0 MB/sec
formatter/numpy/globals.py                 1.00    197.9±0.63µs    14.9 MB/sec    1.02    201.4±0.20µs    14.6 MB/sec
formatter/pydantic/types.py                1.00      4.1±0.01ms     6.2 MB/sec    1.01      4.1±0.01ms     6.2 MB/sec
linter/all-rules/large/dataset.py          1.00     14.1±0.06ms     2.9 MB/sec    1.00     14.2±0.06ms     2.9 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.5±0.02ms     4.8 MB/sec    1.01      3.5±0.03ms     4.7 MB/sec
linter/all-rules/numpy/globals.py          1.00    361.6±0.90µs     8.2 MB/sec    1.01    363.6±1.86µs     8.1 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.2±0.03ms     4.1 MB/sec    1.01      6.2±0.03ms     4.1 MB/sec
linter/default-rules/large/dataset.py      1.00      7.2±0.03ms     5.7 MB/sec    1.00      7.2±0.04ms     5.7 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00   1461.7±1.87µs    11.4 MB/sec    1.00  1465.3±12.86µs    11.4 MB/sec
linter/default-rules/numpy/globals.py      1.00    155.6±0.26µs    19.0 MB/sec    1.00    155.9±0.27µs    18.9 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.2±0.01ms     8.1 MB/sec    1.01      3.2±0.01ms     8.0 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      9.6±0.12ms     4.2 MB/sec    1.03     10.0±0.18ms     4.1 MB/sec
formatter/numpy/ctypeslib.py               1.00      2.1±0.07ms     7.9 MB/sec    1.05      2.2±0.04ms     7.6 MB/sec
formatter/numpy/globals.py                 1.00    234.2±7.15µs    12.6 MB/sec    1.05    245.3±7.10µs    12.0 MB/sec
formatter/pydantic/types.py                1.00      4.7±0.14ms     5.4 MB/sec    1.03      4.8±0.06ms     5.3 MB/sec
linter/all-rules/large/dataset.py          1.00     15.8±0.24ms     2.6 MB/sec    1.00     15.9±0.40ms     2.6 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.2±0.09ms     4.0 MB/sec    1.01      4.2±0.18ms     4.0 MB/sec
linter/all-rules/numpy/globals.py          1.00    503.9±9.88µs     5.9 MB/sec    1.00    505.4±8.34µs     5.8 MB/sec
linter/all-rules/pydantic/types.py         1.02      7.1±0.14ms     3.6 MB/sec    1.00      7.0±0.16ms     3.6 MB/sec
linter/default-rules/large/dataset.py      1.01      8.1±0.12ms     5.0 MB/sec    1.00      8.0±0.05ms     5.1 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01  1727.0±48.36µs     9.6 MB/sec    1.00  1716.8±33.80µs     9.7 MB/sec
linter/default-rules/numpy/globals.py      1.00    200.2±3.69µs    14.7 MB/sec    1.03    206.2±6.99µs    14.3 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.7±0.10ms     7.0 MB/sec    1.00      3.6±0.09ms     7.0 MB/sec

LouisDISPA · 2023-07-08T16:16:26Z

I was looking at the same formatting of Black in other statements, I found lambda a: [expr], yield [expr] and with [expr]: have the same behaviour as discussed here.

The with statement is implemented but has a diff with black (discussed here) and the solution seems to be quite complicated. I don't think I can resolve it, but if a fix is found I could maybe transfer it. I suppose there should be an option for the Expr::format to say "stay inline until there is a parenthesis".

Feel free to close the issue if the diff with black is not acceptable. I will continue to read the code but don't except me to find a solution. Love what you are doing, such an amazing work! Sorry for the inconvenience.

breaks formatting stability

Break format stability

LouisDISPA · 2023-07-10T07:27:26Z

I found some issues with stability, and I don't know how to solve it.

Input:

raise ( # some comment
    # another one
)

First format:

raise ( 
      # some comment
    # another one
)

Second format:

raise ( 
    # some comment
    # another one
)

MichaReiser · 2023-07-10T07:35:54Z

raise ( # some comment
    # another one
)

@konstin what do you think. Should we make the trailing comment a leading comment of the value? Or should it be a dangling comment and we format it manually inside of the raise stmt?

konstin · 2023-07-10T09:30:19Z

i don't have a good intuition, i'm fine with merging whatever is stable. Function call e.g. currently format

a = x( # b
    # c
)

to

a = x()  # b
# c

I think we should add an empty parentheses comment util, unless someone else has specific ideas, i'll prototype something today or tomorrow that keeps both kinds of comments where they are for general empty parentheses

LouisDISPA · 2023-07-10T12:52:33Z

I found the stability issue, it was in the tuple formatting implementation. I pushed a simple fix but if you want I can open a separate PR for the fix.

I copied the FormatExprList implementation to fix it: here

konstin · 2023-07-10T16:06:39Z

I found the stability issue, it was in the tuple formatting implementation. I pushed a simple fix but if you want I can open a separate PR for the fix.

thank you! i'm fine with merging it with this PR with this fix

In general, is this PR ready to be merged?

LouisDISPA · 2023-07-10T17:05:38Z

I found the stability issue, it was in the tuple formatting implementation. I pushed a simple fix but if you want I can open a separate PR for the fix.

thank you! i'm fine with merging it with this PR-

In general, is this PR ready to be merged?

Yes I'm putting this PR in ready status.

I also found the same issue with FormatExprDict formatting but I will open a different PR, I'm trying to find a cleaner way for handling dangling comments and share the implementation.

konstin · 2023-07-10T19:23:45Z

I also found the same issue with FormatExprDict formatting but I will open a different PR, I'm trying to find a cleaner way for handling dangling comments and share the implementation.

thanks, that would be great!

MichaReiser reviewed Jul 7, 2023

View reviewed changes

MichaReiser mentioned this pull request Jul 7, 2023

📋 Formatter: Syntax Formatting #4798

Closed

72 tasks

konstin added the formatter Related to the formatter label Jul 7, 2023

LouisDISPA and others added 4 commits July 8, 2023 18:06

Format raise statement

a224c02

Remove assertion_line

b2abed5

Add raise formatting tests

d2dbd56

Add comments to raise ruff tests

9696633

LouisDISPA force-pushed the main branch from 686493f to 9696633 Compare July 8, 2023 16:06

Louis Dispa added 2 commits July 9, 2023 21:49

Add comments tests

ee373e6

breaks formatting stability

Add comment tests

3f1b489

Break format stability

LouisDISPA marked this pull request as draft July 10, 2023 07:22

Louis Dispa added 4 commits July 10, 2023 14:38

Derive debug for Comments struct

5e15e4f

Add more comment tests

e9886c7

Fix tuple stability

db1f3e2

Add tuple stability fix tests

36614c0

Louis Dispa added 2 commits July 10, 2023 14:53

Formatting and clippy

23ef6d4

Merge remote-tracking branch 'astral/main'

932cc7e

Fix clean comment tests

e8ad2b6

LouisDISPA marked this pull request as ready for review July 10, 2023 17:05

konstin approved these changes Jul 10, 2023

View reviewed changes

konstin merged commit e7e2f44 into astral-sh:main Jul 10, 2023

cnpryer mentioned this pull request Jul 10, 2023

Format delete statement #5169

Merged

4 tasks

MichaReiser mentioned this pull request Jul 20, 2023

Formatter: Yield / YieldFrom (good first issue) #5916

Closed

2 tasks

Conversation

LouisDISPA commented Jul 7, 2023

Summary

Test Plan

Uh oh!

MichaReiser left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaReiser Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

LouisDISPA Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

LouisDISPA Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

zanieb Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

LouisDISPA Jul 8, 2023

Choose a reason for hiding this comment

Uh oh!

LouisDISPA Jul 8, 2023

Choose a reason for hiding this comment

Uh oh!

MichaReiser Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

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!

LouisDISPA commented Jul 8, 2023

Uh oh!

LouisDISPA commented Jul 10, 2023

Uh oh!

MichaReiser commented Jul 10, 2023

Uh oh!

konstin commented Jul 10, 2023

Uh oh!

LouisDISPA commented Jul 10, 2023

Uh oh!

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

Uh oh!

Uh oh!

LouisDISPA commented Jul 10, 2023

Uh oh!

konstin commented Jul 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MichaReiser left a comment •

edited

Loading

github-actions bot commented Jul 7, 2023 •

edited

Loading

konstin commented Jul 10, 2023 •

edited

Loading