-
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
Use parenthesized_with_dangling_comments
in arguments formatter
#6376
Conversation
@@ -103,7 +101,12 @@ impl FormatNodeRule<Arguments> for FormatArguments { | |||
// ) | |||
// ``` | |||
// TODO(konstin): Doesn't work see wrongly formatted test |
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.
@konstin - Is this still applicable? I can't tell what it applies to.
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.
i don't think think so
PR Check ResultsBenchmarkLinux
Windows
|
e92bad8
to
9b61f73
Compare
The next stability failure (i.e., the reason this PR is still failing) is fixed in #6380. |
parenthesized_with_dangling_comments( | ||
"(", | ||
dangling_comments, | ||
&group(&all_arguments), | ||
")" | ||
) |
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: Can we change the builder to parenthesized("(", &group(&all_arguments)), ")").with_dangling_comments(dangling_comments)
to be more in line with how other builders work?
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.
i'm thinking about parenthesized
and parenthesized_without_dangling_comments
since the latter should be the exception
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.
I will handle separately since it would be unrelated to this change.
@@ -103,7 +101,12 @@ impl FormatNodeRule<Arguments> for FormatArguments { | |||
// ) | |||
// ``` | |||
// TODO(konstin): Doesn't work see wrongly formatted test |
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.
i don't think think so
parenthesized_with_dangling_comments( | ||
"(", | ||
dangling_comments, | ||
&group(&all_arguments), | ||
")" | ||
) |
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.
i'm thinking about parenthesized
and parenthesized_without_dangling_comments
since the latter should be the exception
…tral-sh#6376) ## Summary Fixes an instability whereby this: ```python def get_recent_deployments(threshold_days: int) -> Set[str]: # Returns a list of deployments not older than threshold days # including `/root/zulip` directory if it exists. recent = set() threshold_date = datetime.datetime.now() - datetime.timedelta( # noqa: DTZ005 days=threshold_days ) ``` Was being formatted as: ```python def get_recent_deployments(threshold_days: int) -> Set[str]: # Returns a list of deployments not older than threshold days # including `/root/zulip` directory if it exists. recent = set() threshold_date = ( datetime.datetime.now() - datetime.timedelta(days=threshold_days) # noqa: DTZ005 ) ``` Which was in turn being formatted as: ```python def get_recent_deployments(threshold_days: int) -> Set[str]: # Returns a list of deployments not older than threshold days # including `/root/zulip` directory if it exists. recent = set() threshold_date = ( datetime.datetime.now() - datetime.timedelta(days=threshold_days) # noqa: DTZ005 ) ``` The second-to-third formattings still differs from Black because we aren't taking the line suffix into account when splitting (astral-sh#6377), but the first formatting is correct and should be unchanged (i.e., the first-to-second formattings is incorrect, and fixed here). ## Test Plan `cargo run --bin ruff_dev -- format-dev --stability-check ../zulip`
Summary
Fixes an instability whereby this:
Was being formatted as:
Which was in turn being formatted as:
The second-to-third formattings still differs from Black because we aren't taking the line suffix into account when splitting (#6377), but the first formatting is correct and should be unchanged (i.e., the first-to-second formattings is incorrect, and fixed here).
Test Plan
cargo run --bin ruff_dev -- format-dev --stability-check ../zulip