Replace black with ruff-format #15312
Conversation
mroeschke
added
Python
| f"{string.ascii_lowercase[i]}": ["sum", "mean", "count"] | ||
| for i in range(6) | ||
| }, | ||
| {f"{string.ascii_lowercase[i]}": ["sum", "mean", "count"] for i in range(6)}, |
There was a problem hiding this comment.
I find the previous version more readable, do we have to configure any line length settings to achieve that?
There was a problem hiding this comment.
I can re-configure ruff to go back to a 79 line length
There was a problem hiding this comment.
Of note: Some of our copyright lines lines are longer than 79 line length, so I had to noqa those.
There was a problem hiding this comment.
Thanks! I think noqa'ing those is fine.
There was a problem hiding this comment.
Adding noqa here isn't ideal. The SPDX identifiers are meant to be machine-readable...
There was a problem hiding this comment.
This is a documented limitation:
astral-sh/ruff#4429
astral-sh/ruff#5899
There was a problem hiding this comment.
The reason I hesitate on this is that I think we are supposed to migrate towards SPDX identifiers in all our copyright headers. We shouldn’t noqa every file.
There was a problem hiding this comment.
FWIW I would also be OK with a line-length of 88 (that's what we use in pandas)
There was a problem hiding this comment.
We can, as the ruff FAQs suggest, run ruff format but ignore E501 (line length issues). This might lead to some lines that are over the line length, but with the conservative value of 79, I don't think it is necessarily that problematic.
There was a problem hiding this comment.
Good idea with ignoring E501. Yeah we can go with that for now.
python/cudf/cudf/core/resample.py
Outdated
| @@ -1,4 +1,4 @@ | |||
| # SPDX-FileCopyrightText: Copyright (c) 2021-2024, NVIDIA CORPORATION & AFFILIATES. | |||
| # SPDX-FileCopyrightText: Copyright (c) 2021-2024, NVIDIA CORPORATION & AFFILIATES. # noqa: E501 | |||
There was a problem hiding this comment.
I don't really want to make linter exceptions for the copyrights "inline". 😕 What alternatives do we have? A line length of 88 would permit this.
There was a problem hiding this comment.
Almost everywhere else (which is why we don't need it), we have NVIDIA CORPORATION rather than NVIDIA CORPORATION & AFFILIATES.
Not saying which is right/wrong, just noting.
There was a problem hiding this comment.
For these files, it needs to stay as-is with the "& AFFILIATES".
python/cudf/cudf/core/resample.py
Outdated
| @@ -1,4 +1,4 @@ | |||
| # SPDX-FileCopyrightText: Copyright (c) 2021-2024, NVIDIA CORPORATION & AFFILIATES. | |||
| # SPDX-FileCopyrightText: Copyright (c) 2021-2024, NVIDIA CORPORATION & AFFILIATES. # noqa: E501 | |||
There was a problem hiding this comment.
Almost everywhere else (which is why we don't need it), we have NVIDIA CORPORATION rather than NVIDIA CORPORATION & AFFILIATES.
Not saying which is right/wrong, just noting.
Co-authored-by: Lawrence Mitchell <[email protected]>
Co-authored-by: Bradley Dice <[email protected]>
|
/merge |
since #15312 moved formatting from Black to Rufft, it would make sense also unify import formatting under the same ruff so use build-in `I` rule instead of additional `isort` Authors: - Jirka Borovec (https://github.com/Borda) - Kyle Edwards (https://github.com/KyleFromNVIDIA) - Bradley Dice (https://github.com/bdice) Approvers: - Bradley Dice (https://github.com/bdice) - Vyas Ramasubramani (https://github.com/vyasr) - https://github.com/jakirkham URL: #16685
Description
xref #14882
This PR replaces
blackwithruff-formatwith it's default configurations. The ruff configuration had a line length of 88 while black had a line length configuration of 79, so aligned them to 79.The next step would be to consider replacing
isorttooChecklist