flake8
failed.
JSHint
passed.
Discussion: auto-formatting (diffviewer).
Review Request #13380 — Created Oct. 26, 2023 and discarded
This is here to facilitate our discussion about potentially adopting
auto-formatting for our codebase. This change usesblackto format the
diffviewerapp with the following settings:[tool.black] line-length = 80 target-version = ['py38', 'py39', 'py310', 'py311', 'py312'] skip-string-normalization = true
| Summary | ID |
|---|---|
| ddb4fa0cf74b8e086a6c52fdfae1f41021d4b7e6 |
| Description | From | Last Updated |
|---|---|---|
|
This sort of thing goes against the purpose of the multi-line formatting here. We are trying to create a list … |
 chipx86
|
|
|
This gives us less room per line to work with localized strings. It's not the worst thing in the world, … |
chipx86
|
|
|
I'm confused as to why it chose this presentation. There are other examples like this, but it seems inconsistent. My … |
chipx86
|
|
|
I don't mind this format when we're doing type hints, and think it provides a lot of value there. In … |
chipx86
|
|
|
line too long (80 > 79 characters) Column: 80 Error code: E501 |
 reviewbot
|
|
|
I'm fully against this. The multi-line format is far more readable than what Black is choosing to produce here, and … |
chipx86
|
|
|
This particular conditional was not great, and I understand what Black is trying to do here with the whole "the … |
chipx86
|
|
|
line break before binary operator Column: 13 Error code: W503 |
reviewbot
|
|
|
line break before binary operator Column: 13 Error code: W503 |
reviewbot
|
|
|
line break before binary operator Column: 13 Error code: W503 |
reviewbot
|
|
|
line break before binary operator Column: 13 Error code: W503 |
reviewbot
|
|
|
line break before binary operator Column: 13 Error code: W503 |
reviewbot
|
|
|
line break before binary operator Column: 13 Error code: W503 |
reviewbot
|
|
|
line break before binary operator Column: 17 Error code: W503 |
reviewbot
|
|
|
whitespace before ':' Column: 30 Error code: E203 |
reviewbot
|
|
|
Huh, this seems like a strange thing for Black to insist upon. I don't think it buys us anything at … |
chipx86
|
|
|
whitespace before ':' Column: 30 Error code: E203 |
reviewbot
|
|
|
line break before binary operator Column: 13 Error code: W503 |
reviewbot
|
|
|
line break before binary operator Column: 13 Error code: W503 |
reviewbot
|
|
|
This is similar to the yield self.new_chunk up above. Short of using keyword arguments and multiple lines, I don't think … |
chipx86
|
|
|
line break before binary operator Column: 13 Error code: W503 |
reviewbot
|
|
|
line break before binary operator Column: 13 Error code: W503 |
reviewbot
|
|
|
line break before binary operator Column: 21 Error code: W503 |
reviewbot
|
|
|
line break before binary operator Column: 21 Error code: W503 |
reviewbot
|
|
|
line break before binary operator Column: 21 Error code: W503 |
reviewbot
|
|
|
line break before binary operator Column: 17 Error code: W503 |
reviewbot
|
|
|
line break before binary operator Column: 17 Error code: W503 |
reviewbot
|
|
|
line break before binary operator Column: 13 Error code: W503 |
reviewbot
|
|
|
line break before binary operator Column: 13 Error code: W503 |
reviewbot
|
|
|
line break before binary operator Column: 17 Error code: W503 |
reviewbot
|
|
|
line too long (80 > 79 characters) Column: 80 Error code: E501 |
reviewbot
|
|
|
line break before binary operator Column: 17 Error code: W503 |
reviewbot
|
|
|
line break before binary operator Column: 17 Error code: W503 |
reviewbot
|
|
|
line break before binary operator Column: 21 Error code: W503 |
reviewbot
|
|
|
line break before binary operator Column: 25 Error code: W503 |
reviewbot
|
|
|
line break before binary operator Column: 25 Error code: W503 |
reviewbot
|
|
|
line break before binary operator Column: 25 Error code: W503 |
reviewbot
|
|
|
line break before binary operator Column: 17 Error code: W503 |
reviewbot
|
-
I only went through a handful of the files, given the size of the diff viewer code.
There are things I like about what this does. Lots of good improvements to some of the code. But then there are things I see that are deal-breakers to me. Too many "This is the favorite style of the Black team" changes that I don't see absolute good reasons for, beyond preference, and areas where I strongly feel it makes our code less-readable. I've noted some of these.
If we had a formatter we could control, I'd be okay with code formatting. I like consistency, and I like readable code. But I'm not on board with what Black has produced here.
-
This sort of thing goes against the purpose of the multi-line formatting here. We are trying to create a list of file extensions where we'd block styling. That is only one right now, but, like a trailing comma in a dictionary, this was formatted to reduce code changes as we make additions.
The inline comment should have been a clue to Black, I'd have hoped, but now that comment looks to apply to the variable and not the value.
-
This gives us less room per line to work with localized strings. It's not the worst thing in the world, but I don't think it's a good tradeoff.
-
I'm confused as to why it chose this presentation. There are other examples like this, but it seems inconsistent. My guess is, it was done because of the variables taking multiple lines, but now it's actually collapsed the variable to one line and added a third, but it doesn't buy us anything.
The correct thing here would be to use keyword arguments. That's not Black's job to know, I get that, but I think this is a case where Black is not improving things, it's merely changing things.
-
I don't mind this format when we're doing type hints, and think it provides a lot of value there. In this case, I don't think it provides value. If it were one parameter per line, fine, but this is another "the Black developers like this" style that I think buys us nothing here.
-
I'm fully against this. The multi-line format is far more readable than what Black is choosing to produce here, and I'd like to stick with that. I could read that far better than what it turned this into.
-
This particular conditional was not great, and I understand what Black is trying to do here with the whole "the nested code is visually aligned with the conditional" situation. But I will never like this for
ifstatements. It's a choice for sure, but noe one I'm on board with. -
Huh, this seems like a strange thing for Black to insist upon. I don't think it buys us anything at all.
-
This is similar to the
yield self.new_chunkup above. Short of using keyword arguments and multiple lines, I don't think this format really buys us anything, and is another "pep8-tool-ism" that I don't like.
