-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 StmtWith #5350
format StmtWith #5350
Conversation
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinux
Windows
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. Thank you! I've one suggestion on how to format the with items that span over multiple lines. But this is otherwise good to go.
with (a): # should remove brackets | ||
... | ||
|
||
# TODO: black doesn't wrap this, but maybe we want to anyway? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds reasonable to me. We should make it wrap the same as for function definition
# All flat
def test(a, b, c):
pass
# parentheses expanded
def test(
a, b, c
):
pass
# all expanded
def test(
a,
b,
c
):
pass
You can achieve this by using two nested groups:
- outer group: For the parentheses and indent
- inner group: Around the with items
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh wait. when there is an as
, wrapping isn't allowed in python < 3.9 (3.8 isn't eol until oct '24)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can change that when adding support for targeting different python versions. It's something that we'll need to add to the formatter options.
|
||
with call(arg): | ||
call(arg) | ||
# fmt: skip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this fmt: skip
intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, this whole block is leftover from debugging. will remove
let dangling_comments = comments.dangling_comments(item.as_any_node_ref()); | ||
let trailing_items_comments = body.first().map(|body_start| { | ||
let before_body = dangling_comments | ||
.partition_point(|comment| comment.slice().end() < body_start.start()); | ||
|
||
let (trailing_items_comments, rest) = dangling_comments.split_at(before_body); | ||
debug_assert!(rest.is_empty()); | ||
trailing_items_comments | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the partition_point
or can we format all dangling_comments
at once? Because it seems there's only a single place where dangling comments can appear (and thus, splitting is unnecessary).
group(&format_args![ | ||
if_group_breaks(&text("(")), | ||
soft_block_indent(&joined_items), | ||
if_group_breaks(&text(")")), | ||
]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: You can use optional_parentheses(&joined_items)
if this lands after #5328
8ef512f
to
2458c3f
Compare
without one some (unusual) with statements had unstable formatting
2458c3f
to
cec170a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Summary
format
StmtWith
(andWithItem
)one outstanding todo (see the bottom of
with.py
)Test Plan
snapshots