-
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
Formatter undocumented deviation: comment on function arguments forces wrapping #7368
Comments
Thanks for the report! We'll need to hear from someone else on the team, but I believe the idea here is that comments should either be regarding:
So I prefer this formatting because it encourages better comment placement. |
In the actual code that inspired this report, the comment is a return self.cached_function(
- *args, **kwargs # type: ignore[arg-type] # might be unhashable
+ *args, # type: ignore[arg-type] # might be unhashable
+ **kwargs, # type: ignore[arg-type] # might be unhashable
) which is not the end of the world but seems suboptimal. |
Thanks for testing the formatter @andersk! |
Ah thanks for the additional context! I believe we have special handling for pragma comments in some places. In this case I remember discussing that it would be good to suggest narrower use of the pragmas as it's rare that you want it to apply to all of the arguments but I'm not sure the formatter should force you to duplicate like this (or break pragmas in general). |
The underlying technical reason for the splitting is that Ruff associates the comment with the We could improve our formatting to test if the last end of line comment is on the same line as the first argument (indicating that it uses the The alternative is that you write this as: return self.cached_function(*args, **kwargs) # type: ignore[arg-type] # might be unhashable Ruff allows this formatting because pragma comments don't count toward the line width. Regarding pragma comments. You can read more about the decision the reasoning in this discussion. The main challenge with pragma comments is that there's a tension between:
I wanted to avoid 1. because it defeats the purpose of a formatter: Having consistent code formatting is the primary goal. We instead change the definition of pragma comments so that they don't count toward the line width do avoid the annoying case where you've written some code, add a pragma comment, and now the whole code re-flows (including your pragma comment) because it now exceeds the line width. You then have to move the comment and, if you're unlucky, it will trigger another reformat... which is very annoying. |
We're going to explore treating these as dangling and implementing special formatting. It's a known deviation, and not blocking for Beta, but we want to at least try to improve it and see how it goes. |
Is this the same issue that's making the https://play.ruff.rs/62d5edc9-46dd-46b8-89bd-bc3f835a7d24?secondary=Format |
@henribru What you're seeing is related to Ruff handling trailing end of line comments and pragma comments differently. Pragma comments are challenging to get right, especially when migrating between the formatters. I expect them to be less of a problem when you only use ruff (or black, to the matter) |
That makes more sense, but the descriptions there arguably don't cover my case, since the first one says "This deviation only impacts unformatted code, in that Ruff's output should not deviate for code that has already been formatted by Black." and the second talks about Ruff not moving pragma comments. Perhaps those descriptions need to be expanded a bit. |
I encountered the same issue with def read_csv_file(
sensor_fpath, start, end, header, timezone, convention # noqa: ARG001
):
pass One could indeed argue that wrapping encourages narrower use of the pragma, but then it would be consequent to apply it also for other cases, like, e.g., the following example (which is left as is by both black and ruff): def read_csv_file(sensor_fpath, start, end, header, timezone): # noqa: ARG001
pass In my view, wrapping is undesired in both cases. Encouraging "narrower" use of pragma comments makes sense and could e.g. become a lint rule, but I don't think it should determine the wrapping behaviour of the formatter. FYI, this is the only deviation I encountered (twice) when migrating four projects from black to ruff. |
Could it be added to the documented known deviations in the meantime? Or is that only intended for deviations that aren't expected to change? |
We could implement a similar logic to #8431. Except that the ruff/crates/ruff_python_formatter/src/other/arguments.rs Lines 106 to 107 in f4c7bff
Note:
One issue I see come up is that call formatting, arrays, etc would be different from parameter formatting: def func(
a, b, c, d # a
): ... We may need to make the same change for parameters if we want to be consistent. |
I think it's something I'm hitting now I was using the lint for some time, and add lots of noqa comments, - def compare_table_data(self, expected_table_data: list[dict[str, str]], table_name: str | None = None, # noqa: PLR0913
- node: ScyllaNode = None, ignore_order: bool = True, consistent_read: bool = True,
- table_data: list[dict[str, str]] | None = None, **kwargs) -> DeepDiff:
+ def compare_table_data(
+ self,
+ expected_table_data: list[dict[str, str]],
+ table_name: str | None = None, # noqa: PLR0913
+ node: ScyllaNode = None,
+ ignore_order: bool = True,
+ consistent_read: bool = True,
+ table_data: list[dict[str, str]] | None = None,
+ **kwargs,
+ ) -> DeepDiff: I fixed it with the linter |
@fruch this sounds related but is slightly different because the comment isn't at the end of the arguments line (it's after a specific argument). Unfortunately, it's challenging for the formatter to know to which node a suppression comment belongs without running the linter, which would be extremely slow. Can you try using the ruff check --select=RUF100 <path> And apply the fixes with ruff check --fix --select=RUF100 <path> |
Input, unchanged under Black:
Ruff:
The text was updated successfully, but these errors were encountered: