Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 15 commits into from Aug 21, 2023

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

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

@charliermarsh charliermarsh added the formatter Related to the formatter label Aug 16, 2023
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

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

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 marked this pull request as ready for review August 18, 2023 13:39
@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

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

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
Copy link
Member

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

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
17 checks passed
@tjkuson tjkuson deleted the single-line-ellipsis branch August 29, 2023 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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