Suppression Comments #6338
Replies: 6 comments 6 replies
-
|
FWIW, as I communicated in DMs, I'm supportive of this proposal. Am I right that there's nothing in the proposed API that would prevent us from expanding to support suppression-with-expressions in the future? (I'm ignoring the implementation cost and details -- I'm just confirming that there's nothing in the API that precludes us from adding that later if needed.) |
Beta Was this translation helpful? Give feedback.
-
|
I don't see anything that would prevent us from supporting expression-level expressions in the future, beyond that, I don't know how to implement this in a sane way, and that I find expression level suppression comments unintuitive ;) |
Beta Was this translation helpful? Give feedback.
-
|
Generally I am in support of this, it sounds great. Are the following cases supported? [ # fmt: skip
...
]and [...] # fmt: skipShould
Where would they be considered missing? |
Beta Was this translation helpful? Give feedback.
-
No, this would not be supported because the comment is in the middle of a statement (expression even). Is this a pattern that you use frequently?
This is supported for as long the expression is at the end of the statement: # Supported, suppresses formatting of the whole statement
[ ... ] # fmt: skip
a = [...] # fmt: skip
# Unsupported
a = (
[ ... ] # fmt: skip -> In the middle of an expression
)
I'm leaning towards promoting
I would still recommend supporting
I wouldn't warn if we decide to support both (which is what I'm proposing)
Yes, the same as we should warn about useless |
Beta Was this translation helpful? Give feedback.
-
To extend on why I propose not to allow additional places where Ideally, we come up with a short and precise definition for both. And that's where I believe we would struggle if we allow Edit: Yet another reason is that I think it's unintuitive what a, b = [ # fmt: skip
...
]Does the |
Beta Was this translation helpful? Give feedback.
-
Not necessarily but that's how
It might be nice to support this. It doesn't seem ambiguous as to what is being excluded here. Are there ambiguous cases?
+1 to that |
Beta Was this translation helpful? Give feedback.
-
I'm not sure. Pragma comments are still a mystery to me.
Not in this specific example, but it's probably rare to have a parenthesized list expression. It's more likely that you have something like this: a = {
"key": [...] # fmt: skip
}
# Or
```python
a = (
1 +
[ 2, 3, 4 ] # fmt: skip
)Here I would expect that a = (
1 + [2, 3, 4] # fmt: skip
)be a valid formatting? It's possible from a technical standpoint to support (
a
+ b # fmt: skip
+ c
)This suppression happens to be in the middle of the |
Beta Was this translation helpful? Give feedback.
-
|
To sum the discussion up to this point:
|
Beta Was this translation helpful? Give feedback.
-
|
Would like to request that |
Beta Was this translation helpful? Give feedback.
-
|
I appreciate opinion will be divided over when and if arrays deserve manual layout, but trying out ruff 0.1.3, I see it does not obey examples like this (and related hand-tuned arrays formatted for element alignment): self.assertTrue(
np.array_equal(
alignment.indices,
np.array(
[
# fmt: off
[0, 1, 2, 3, 4, 5, 6, 7, -1, 8, 9, 10],
[0, -1, 1, -1, 2, 3, -1, 4, 5, 6, -1, -1],
# fmt: on
]
),
)
)Referring to the documentation above, I see the suggested alternatives for use with
# Up to user to decide how compact to make this array:
expected = np.array([
[0, 1, 2, 3, 4, 5, 6, 7, -1, 8, 9, 10],
[0, -1, 1, -1, 2, 3, -1, 4, 5, 6, -1, -1],
]) # fmt: skip
self.assertTrue(np.array_equal(alignment.indices, expected))I'm guessing your preferred alternative for this use-case is the temporary variable? (This example is actually a slight simplification as we also told flake8 to ignore the spaces; examples like this constitute the majority of the deviations from black in the repository concerned) |
Beta Was this translation helpful? Give feedback.
-
|
Hy @peterjc Yes, I think you covered all options with the corresponding tradeoffs. I agree; these all feel less ergonomic than what Black and am sorry for that. We have multiple options from here:
Since we have limited time, I prefer to invest our time into the second option as soon as we finish our stabilization work on the formatter. The reasoning is that having to use Regarding your original question. I would probably suppress either the whole statement or extract the variable as you suggested. |
Beta Was this translation helpful? Give feedback.
-
|
I didn't see table-driven tests with It turns out the trailing @pytest.mark.parametrize(
["which_result", "find_uv_bin_result", "expected"],
[
("/usr/bin/uv", UV_IN_PIPX_VENV, (True, "/usr/bin/uv")),
("/usr/bin/uv", FileNotFoundError, (True, "/usr/bin/uv")),
(None, UV_IN_PIPX_VENV, (True, UV_IN_PIPX_VENV)),
(None, FileNotFoundError, (False, "uv")),
],
) # fmt: skip
def test_find_uv(monkeypatch, which_result, find_uv_bin_result, expected):
... |
Beta Was this translation helpful? Give feedback.
-
This discussion proposes a design for suppression-comments in the formatter and documents the decision for the future.
Black
Black supports
fmt: offfollowed by an optionalfmt: onown-line comments. Anything between does not get formatted:The formatting gets automatically re-enabled if there's a dedent decreasing the indention level below the initial indentation of the
fmt: offcomment. The following is semantically identical to the above example:The indentation behavior doesn't apply to parenthesized expressions (any expression enclosed by a
(),[], or{}pair). Instead, Black re-enables the formatting when reaching the opening parentheses, meaning the following two examples are semantically equivalent:Black further supports trailing
fmt: skipcomments that disable formatting for the preceding line. I haven't been able to figure out fully what the logic is when using inside of expressions. Here a few examples:Proposal
Implementing Black's very flexible
fmt: off..fmt: onsuppression comments is challenging with our formatter architecture, and I also don't think it is necessary. I analyzed the usages of our ecosystem projects, and the vast majority only use statement-level suppression comments. These are easy to reason about and almost always sufficient.Suppression comments are also a sign that the formatter isn't good enough. I want to focus on improving formatting rather than investing in a very flexible and powerful suppression system. Suppression comments don't just mean that we're back to discussing how the suppressed code should be formatted, but the suppression also decreases readability overall.
What's important from my perspective is that:
Own line
fmt: offandfmt: oncommentsfmt: offandfmt: oncomments allow to disable formatting for a range of statements. The statements must have the same or higher indentation level than the statement annotated with thefmt: offcomment. Thefmt: onis optional, but its best practice to use it.I propose to limit
fmt: off... andfmt: onto statement level only. The following examples are all validHowever, the following examples are not valid (but are valid in black)
if..elifcase doesn't fulfill the goal that suppression comments should be intuitive. Does it suppress theelifbody only or all statements with the same indent as theelifcase?That's why I believe
fmt: skipis usefulTrailing
fmt: skiporfmt: offcommentsA trailing
fmt: skiporfmt: offcomment suppresses formatting for the preceding statement, case header, decorator, function definition, or class definition:There are two caveats where the use of
fmt: skipis not intuitive:a; b; c # fmt skipsuppresses the formatting for the statementc, and not the whole physical line as one might expect. This is because the AST has no sequence statement.def test(): pass # fmt: skipsuppresses formatting for the whole function and not just thepassstatement.I think this is a shortcoming that we have to accept.
Unsupported Use cases
I went through all
fmt: offandfmt: skipusages to identify the uses that either require changing the suppression comments or are entirely unsupported.Between decorators
-> Use a trailing
fmt: skipInside Lists
Either use
fmt: skip, or assign the list to a temporary variable and suppress formatting for the assignment statement.Inside assertions
I've seen both
fmt: offcomments on the statement and expression level. Disallowingfmt: offon the expression level increases consistency.If Conditions
Inside dictionaries
This seems useful but won't be supported anymore.
Observation
The main use case for suppression comments is when it comes to formatting lists. We should improve list formatting by e.g. using a fill layout when all expressions in a list are simple expressions (numbers, simple strings). The fill layout tries to fit as many items as possible on a line rather than putting each item on its own line.
Open Question
fmt: oncomments?Beta Was this translation helpful? Give feedback.
All reactions