-
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
Insert trailing comma when function breaks with single argument #8921
Conversation
|
36b90a4
to
c0f5f67
Compare
This leads to decreased similarity in most cases (but the test fixtures show improvement). I'm pretty lost on what Black's logic is for inserting the trailing comma vs. not. For example, Black adds a trailing comma to the second example here, but not the first... def _start_listening(
ws_event_callback: Callable[str, int]
) -> List[example.ExampleConfig]:
pass
def _start_listening(
ws_event_callback: Callable[List[str]]
) -> List[example.ExampleConfig]:
pass |
+1 for consistency |
I'm now leaning towards proceeding with this change. My best guess is that Black omits the trailing comma if the annotation contains a comma, which seems incorrect to me. |
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.
+1 for the consistency, since it changes a bunch of files and decreases black compatibility, do we want to guard it behind e.g. preview?
Another idea is that we could only add the trailing comma if the argument itself breaks... So: # Add a trailing comma here...
def _start_listening(
ws_event_callback: List[
CallableButImagineItIsVeryLong[str, int]
],
) -> List[example.ExampleConfig]:
pass
# But not here...
def _start_listening(
ws_event_callback: Callable[List[str]]
) -> List[example.ExampleConfig]:
pass This would still be a deviation from Black in some cases, but it would avoid adding the trailing comma in the event that more arguments are added (since those arguments would stay on the same line). Eh, on second thought, I don't see why that would be better. (Filed an issue on: psf/black#4080) |
I think this is a bug and we don't need to gate it with preview; perhaps if the formatter was not in beta, I would feel differently. |
…9076) ## Summary In #8921, we changed our parameter formatting behavior to add a trailing comma whenever a single-argument function breaks. This introduced a deviation in the case that a function contains a single argument, but _also_ includes a positional-only or keyword-only separator. Closes #9074.
Summary
Given:
We should be inserting a trailing comma after the argument (as long as it's a single-argument function). This was an inconsistency with Black, but also led to some internal inconsistencies, whereby we added the comma if the argument contained a trailing end-of-line comment, but not otherwise.
Closes #8912.
Test Plan
cargo test
Before:
After: