Skip to content

Comments

Format function and class definitions into a single line if its body is an ellipsis#6592

Merged
MichaReiser merged 15 commits intoastral-sh:mainfrom
tjkuson:single-line-ellipsis
Aug 21, 2023
Merged

Format function and class definitions into a single line if its body is an ellipsis#6592
MichaReiser merged 15 commits intoastral-sh:mainfrom
tjkuson:single-line-ellipsis

Conversation

@tjkuson
Copy link
Contributor

@tjkuson tjkuson commented Aug 15, 2023

Summary

Formats

class A:
    ...

to

class A: ...

Closes #5822.

Test Plan

cargo test

@tjkuson tjkuson marked this pull request as ready for review August 15, 2023 10:38
@github-actions
Copy link
Contributor

github-actions bot commented Aug 15, 2023 •
edited
Loading

PR Check Results

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      3.3±0.02ms    12.2 MB/sec    1.00      3.3±0.03ms    12.3 MB/sec
formatter/numpy/ctypeslib.py               1.00    671.3±7.06µs    24.8 MB/sec    1.00    674.7±6.34µs    24.7 MB/sec
formatter/numpy/globals.py                 1.00     69.1±0.39µs    42.7 MB/sec    1.00     69.3±0.43µs    42.6 MB/sec
formatter/pydantic/types.py                1.00  1310.0±13.27µs    19.5 MB/sec    1.01  1324.9±14.02µs    19.2 MB/sec
linter/all-rules/large/dataset.py          1.00     11.1±0.04ms     3.7 MB/sec    1.00     11.1±0.04ms     3.7 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      3.0±0.01ms     5.5 MB/sec    1.00      3.0±0.03ms     5.5 MB/sec
linter/all-rules/numpy/globals.py          1.03    337.1±1.20µs     8.8 MB/sec    1.00    326.5±1.11µs     9.0 MB/sec
linter/all-rules/pydantic/types.py         1.00      5.8±0.02ms     4.4 MB/sec    1.00      5.8±0.02ms     4.4 MB/sec
linter/default-rules/large/dataset.py      1.02      5.8±0.02ms     7.0 MB/sec    1.00      5.7±0.02ms     7.1 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01   1208.3±5.44µs    13.8 MB/sec    1.00   1194.0±8.99µs    13.9 MB/sec
linter/default-rules/numpy/globals.py      1.00    124.7±0.44µs    23.7 MB/sec    1.01    125.4±5.41µs    23.5 MB/sec
linter/default-rules/pydantic/types.py     1.01      2.6±0.01ms     9.9 MB/sec    1.00      2.5±0.01ms    10.1 MB/sec

Windows

group                                      main                                    pr
-----                                      ----                                    --
formatter/large/dataset.py                 1.02      4.6±0.28ms     8.8 MB/sec     1.00      4.5±0.20ms     9.0 MB/sec
formatter/numpy/ctypeslib.py               1.02   964.0±67.52µs    17.3 MB/sec     1.00   947.7±44.68µs    17.6 MB/sec
formatter/numpy/globals.py                 1.04     99.4±6.72µs    29.7 MB/sec     1.00     95.5±4.08µs    30.9 MB/sec
formatter/pydantic/types.py                1.01  1937.5±114.58µs    13.2 MB/sec    1.00  1917.2±156.22µs    13.3 MB/sec
linter/all-rules/large/dataset.py          1.00     16.5±0.49ms     2.5 MB/sec     1.01     16.6±0.45ms     2.5 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.5±0.20ms     3.7 MB/sec     1.00      4.5±0.18ms     3.7 MB/sec
linter/all-rules/numpy/globals.py          1.02   569.9±39.92µs     5.2 MB/sec     1.00   560.9±37.18µs     5.3 MB/sec
linter/all-rules/pydantic/types.py         1.02      8.6±0.33ms     3.0 MB/sec     1.00      8.5±0.34ms     3.0 MB/sec
linter/default-rules/large/dataset.py      1.00      9.0±0.30ms     4.5 MB/sec     1.02      9.2±0.36ms     4.4 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1912.2±80.18µs     8.7 MB/sec     1.08      2.1±0.13ms     8.1 MB/sec
linter/default-rules/numpy/globals.py      1.01   228.3±14.48µs    12.9 MB/sec     1.00   225.5±12.04µs    13.1 MB/sec
linter/default-rules/pydantic/types.py     1.00      4.1±0.15ms     6.3 MB/sec     1.01      4.1±0.17ms     6.2 MB/sec

@MichaReiser MichaReiser mentioned this pull request Aug 15, 2023
Merged
MichaReiser
MichaReiser reviewed Aug 15, 2023
View reviewed changes
@charliermarsh charliermarsh added the formatter Related to the formatter label Aug 16, 2023
MichaReiser
MichaReiser requested changes Aug 17, 2023
View reviewed changes
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.

This starts to looking good. I left a few comments that hopefully help to solve the TODOs. We have to use the new clause_body everywhere where we do block_indent(&body.format()) today (all compound statements). But we can do this once the class and function formatting is stable.

@tjkuson tjkuson marked this pull request as draft August 18, 2023 11:42
@MichaReiser
Copy link
Member

MichaReiser commented Aug 18, 2023

Wow, nice work. This shoots our typeshed compatibility up by more than 20%

project similarity index before after
cpython 0.75473 0.75473
django 0.99805 0.99805
transformers 0.99620 0.99620
twine 0.99876 0.99876
typeshed 0.74292 0.99953
warehouse 0.99601 0.99601
zulip 0.99727 0.99727

@tjkuson
Copy link
Contributor Author

tjkuson commented Aug 18, 2023

I'm trying to work out why it formats

class EllipsisWithLeadingComment:
	# leading
	...

to

class EllipsisWithLeadingComment: # leading
...

Once that's sorted, it should be possible to do the same for every other clause body...

@MichaReiser
Copy link
Member

MichaReiser commented Aug 18, 2023

I'm trying to work out why it formats

class EllipsisWithLeadingComment:
	# leading
	...

to

class EllipsisWithLeadingComment: # leading
...

Once that's sorted, it should be possible to do the same for every other clause body...

Sounds good. Let me know if you want any help.

@tjkuson tjkuson force-pushed the single-line-ellipsis branch from 57f6270 to ebea2a9 Compare August 18, 2023 12:55
@tjkuson tjkuson marked this pull request as ready for review August 18, 2023 13:39
@tjkuson tjkuson requested a review from MichaReiser August 18, 2023 13:39
@tjkuson tjkuson force-pushed the single-line-ellipsis branch from a9bf9a0 to 68fe3f1 Compare August 18, 2023 14:47
@tjkuson
Copy link
Contributor Author

tjkuson commented Aug 18, 2023

The logic for match statements seems different to other compound statements

let mut last_case = first;

for case in cases_iter {
    write!(
        f,
        [block_indent(&format_args!(
            leading_alternate_branch_comments(
                comments.leading(case),
                last_case.body.last(),
            ),
            case.format()
        ))]
    )?;
    last_case = case;
}

I'm not quite sure how this would with clause_body.

@MichaReiser
Copy link
Member

MichaReiser commented Aug 18, 2023

Nice, this change is so cool. Thanks for working on it.

Do we need to change the try formatting as well?

The logic for match statements seems different to other compound statements

I understand this isn't needed on the match level (that renders the cases), only on the case level. You can find its implementation here

ruff/crates/ruff_python_formatter/src/other/match_case.rs

Lines 27 to 63 in 0cea497

write!(
f,
[
clause_header(
ClauseHeader::MatchCase(item),
dangling_item_comments,
&format_with(|f| {
write!(f, [text("case"), space()])?;
let leading_pattern_comments = comments.leading(pattern);
if !leading_pattern_comments.is_empty() {
parenthesized(
"(",
&format_args![
leading_comments(leading_pattern_comments),
pattern.format()
],
")",
)
.fmt(f)?;
} else if is_match_case_pattern_parenthesized(item, pattern, f.context())? {
parenthesized("(", &pattern.format(), ")").fmt(f)?;
} else {
pattern.format().fmt(f)?;
}
if let Some(guard) = guard {
write!(f, [space(), text("if"), space(), guard.format()])?;
}
Ok(())
}),
),
block_indent(&body.format())
]
)
}

MichaReiser
MichaReiser reviewed Aug 18, 2023
View reviewed changes
@MichaReiser
Copy link
Member

MichaReiser commented Aug 18, 2023

I think the last thing now is to add the handling to the try statement

ruff/crates/ruff_python_formatter/src/statement/stmt_try.rs

Lines 136 to 143 in 0cea497

write!(
f,
[
clause_header(header, trailing_case_comments, &text(kind.keyword()))
.with_leading_comments(leading_case_comments, previous_node),
block_indent(&body.format())
]
)?;

@MichaReiser MichaReiser merged commit 2a8d24d into astral-sh:main Aug 21, 2023
@tjkuson tjkuson deleted the single-line-ellipsis branch August 29, 2023 10:44
@mwaskom mwaskom mentioned this pull request Feb 5, 2024
Merged
@etienneschalk etienneschalk mentioned this pull request Feb 18, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaReiser MichaReiser MichaReiser approved these changes

Assignees

No one assigned

Labels

formatter Related to the formatter

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Formatter: Ellipses in stub files get formatted on a single line

3 participants

@tjkuson @MichaReiser @charliermarsh