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

Inconsistent removal of trailing comma in single-argument functions #8912

Closed
charliermarsh opened this issue Nov 29, 2023 · 7 comments · Fixed by #8921
Closed

Inconsistent removal of trailing comma in single-argument functions #8912

charliermarsh opened this issue Nov 29, 2023 · 7 comments · Fixed by #8921
Assignees
Labels
bug Something isn't working formatter Related to the formatter

Comments

@charliermarsh
Copy link
Member

See: https://play.ruff.rs/96810c27-3971-44af-8eb5-5b5ebae1668e. We remove the trailing comma in the first case, but not in the second, when the trailing comment is present.

Note that this only happens with "skip-magic-trailing-comma": true. If you set "skip-magic-trailing-comma": false in the example above, we consistently insert the trailing comma.

Originally reported here: #1200 (comment).

@charliermarsh charliermarsh added bug Something isn't working formatter Related to the formatter labels Nov 29, 2023
@MichaReiser MichaReiser added this to the Formatter: Stable milestone Nov 29, 2023
@charliermarsh
Copy link
Member Author

Another way to look at it: https://play.ruff.rs/5bf54dc0-235e-4cee-9902-90eff0a19150

Here, we don't insert a trailing comma, when we should.

@charliermarsh
Copy link
Member Author

This seems tricky to fix because we need to know if the parent group breaks.

@MichaReiser
Copy link
Member

MichaReiser commented Nov 30, 2023

This seems tricky to fix because we need to know if the parent group breaks.

Can you explain in detail or share some IR. You might be able to assign a group id to the parent group and use if_group_breaks(parent_group_id)

This is how it is implemented in biome

The array creates a group:

https://github.com/MichaReiser/biome/blob/59acbfd1e6458b13b5b44d4c5c33ead6a428435e/crates/biome_js_formatter/src/js/expressions/array_expression.rs#L45-L48

and we use it when formatting the trailing separator here

https://github.com/MichaReiser/biome/blob/59acbfd1e6458b13b5b44d4c5c33ead6a428435e/crates/biome_js_formatter/src/js/lists/array_element_list.rs#L42-L44

which ultimately resolves here

https://github.com/MichaReiser/biome/blob/63e4c40051ec15563cc586890c1c350b8c7b948e/crates/biome_formatter/src/separated.rs#L80-L82

But it depends on how the groups are structured in detail. But I thought it might help

@MichaReiser
Copy link
Member

MichaReiser commented Nov 30, 2023

This could also be solved by associating the comments with the arguments. Because it would allow you to move the comment out of the inner group.

Using the parent group seems incorrect to me (which, breaks as well because the child group is breaking) because we want to have a trailing comma for

func(
	a, b, c,
): pass
``

@charliermarsh
Copy link
Member Author

I don't think the comment is relevant. This, for example, needs a trailing comma:

def _example_function_xxxxxxx(
    variable: Optional[List[str]]
) -> List[example.ExampleConfig]:
    pass

@MichaReiser
Copy link
Member

Hmm, but only if it is a single argument function. Could we avoid the inner group if the function only has a single argument? It seems unnecessary to emit two groups in that case.

@charliermarsh
Copy link
Member Author

Maybe? I can try.

@charliermarsh charliermarsh self-assigned this Nov 30, 2023
charliermarsh added a commit that referenced this issue Dec 1, 2023
## Summary

Given:

```python
def _example_function_xxxxxxx(
    variable: Optional[List[str]]
) -> List[example.ExampleConfig]:
    pass
```

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:

| project | similarity index | total files | changed files |

|----------------|------------------:|------------------:|------------------:|
| cpython | 0.75804 | 1799 | 1648 |
| django | 0.99984 | 2772 | 34 |
| home-assistant | 0.99963 | 10596 | 146 |
| poetry | 0.99925 | 317 | 12 |
| transformers | 0.99967 | 2657 | 322 |
| twine | 1.00000 | 33 | 0 |
| typeshed | 0.99980 | 3669 | 18 |
| warehouse | 0.99977 | 654 | 13 |
| zulip | 0.99970 | 1459 | 21 |

After:

| project | similarity index | total files | changed files |

|----------------|------------------:|------------------:|------------------:|
| cpython | 0.75804 | 1799 | 1648 |
| django | 0.99984 | 2772 | 34 |
| home-assistant | 0.99955 | 10596 | 213 |
| poetry | 0.99917 | 317 | 13 |
| transformers | 0.99967 | 2657 | 324 |
| twine | 1.00000 | 33 | 0 |
| typeshed | 0.99980 | 3669 | 18 |
| warehouse | 0.99976 | 654 | 14 |
| zulip | 0.99957 | 1459 | 36 |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working formatter Related to the formatter
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants