Summary

Discussion: auto-formatting (diffviewer).

Review Request #13380 — Created Oct. 26, 2023 and discarded Dec. 26, 2023, 10:39 a.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This is here to facilitate our discussion about potentially adopting
auto-formatting for our codebase. This change uses black to format the
diffviewer app with the following settings:

[tool.black]
line-length = 80
target-version = ['py38', 'py39', 'py310', 'py311', 'py312']
skip-string-normalization = true

Testing Done

Commits

Summary	ID
Discussion: auto-formatting (diffviewer). This is here to facilitate our discussion about potentially adopting auto-formatting for our codebase. This change uses `black` to format the `diffviewer` app with the following settings: ``` [tool.black] line-length = 80 target-version = ['py38', 'py39', 'py310', 'py311', 'py312'] skip-string-normalization = true ```	ddb4fa0cf74b8e086a6c52fdfae1f41021d4b7e6

Issues

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	Oct. 26, 2023, 8:56 p.m.
This gives us less room per line to work with localized strings. It's not the worst thing in the world, …	chipx86	Oct. 26, 2023, 8:56 p.m.
I'm confused as to why it chose this presentation. There are other examples like this, but it seems inconsistent. My …	chipx86	Oct. 26, 2023, 8:56 p.m.
I don't mind this format when we're doing type hints, and think it provides a lot of value there. In …	chipx86	Oct. 26, 2023, 8:56 p.m.
line too long (80 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 26, 2023, 3:23 p.m.
I'm fully against this. The multi-line format is far more readable than what Black is choosing to produce here, and …	chipx86	Oct. 26, 2023, 8:56 p.m.
This particular conditional was not great, and I understand what Black is trying to do here with the whole "the …	chipx86	Oct. 26, 2023, 8:56 p.m.
line break before binary operator Column: 13 Error code: W503	reviewbot	Oct. 26, 2023, 3:23 p.m.
line break before binary operator Column: 13 Error code: W503	reviewbot	Oct. 26, 2023, 3:23 p.m.
line break before binary operator Column: 13 Error code: W503	reviewbot	Oct. 26, 2023, 3:23 p.m.
line break before binary operator Column: 13 Error code: W503	reviewbot	Oct. 26, 2023, 3:23 p.m.
line break before binary operator Column: 13 Error code: W503	reviewbot	Oct. 26, 2023, 3:23 p.m.
line break before binary operator Column: 13 Error code: W503	reviewbot	Oct. 26, 2023, 3:23 p.m.
line break before binary operator Column: 17 Error code: W503	reviewbot	Oct. 26, 2023, 3:23 p.m.
whitespace before ':' Column: 30 Error code: E203	reviewbot	Oct. 26, 2023, 3:23 p.m.
Huh, this seems like a strange thing for Black to insist upon. I don't think it buys us anything at …	chipx86	Oct. 26, 2023, 8:56 p.m.
whitespace before ':' Column: 30 Error code: E203	reviewbot	Oct. 26, 2023, 3:23 p.m.
line break before binary operator Column: 13 Error code: W503	reviewbot	Oct. 26, 2023, 3:23 p.m.
line break before binary operator Column: 13 Error code: W503	reviewbot	Oct. 26, 2023, 3:23 p.m.
This is similar to the yield self.new_chunk up above. Short of using keyword arguments and multiple lines, I don't think …	chipx86	Oct. 26, 2023, 8:56 p.m.
line break before binary operator Column: 13 Error code: W503	reviewbot	Oct. 26, 2023, 3:23 p.m.
line break before binary operator Column: 13 Error code: W503	reviewbot	Oct. 26, 2023, 3:23 p.m.
line break before binary operator Column: 21 Error code: W503	reviewbot	Oct. 26, 2023, 3:23 p.m.
line break before binary operator Column: 21 Error code: W503	reviewbot	Oct. 26, 2023, 3:23 p.m.
line break before binary operator Column: 21 Error code: W503	reviewbot	Oct. 26, 2023, 3:23 p.m.
line break before binary operator Column: 17 Error code: W503	reviewbot	Oct. 26, 2023, 3:23 p.m.
line break before binary operator Column: 17 Error code: W503	reviewbot	Oct. 26, 2023, 3:23 p.m.
line break before binary operator Column: 13 Error code: W503	reviewbot	Oct. 26, 2023, 3:23 p.m.
line break before binary operator Column: 13 Error code: W503	reviewbot	Oct. 26, 2023, 3:23 p.m.
line break before binary operator Column: 17 Error code: W503	reviewbot	Oct. 26, 2023, 3:23 p.m.
line too long (80 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 26, 2023, 3:24 p.m.
line break before binary operator Column: 17 Error code: W503	reviewbot	Oct. 26, 2023, 3:24 p.m.
line break before binary operator Column: 17 Error code: W503	reviewbot	Oct. 26, 2023, 3:24 p.m.
line break before binary operator Column: 21 Error code: W503	reviewbot	Oct. 26, 2023, 3:24 p.m.
line break before binary operator Column: 25 Error code: W503	reviewbot	Oct. 26, 2023, 3:24 p.m.
line break before binary operator Column: 25 Error code: W503	reviewbot	Oct. 26, 2023, 3:24 p.m.
line break before binary operator Column: 25 Error code: W503	reviewbot	Oct. 26, 2023, 3:24 p.m.
line break before binary operator Column: 17 Error code: W503	reviewbot	Oct. 26, 2023, 3:24 p.m.

flake8 failed.

JSHint passed.

flake8

```
Warning: Showing 30 of 80 failures.
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line too long (80 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 13
Error code: W503
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 13
Error code: W503
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 13
Error code: W503
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 13
Error code: W503
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 13
Error code: W503
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 13
Error code: W503
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 17
Error code: W503
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been dropped. Show all issues
```
whitespace before ':'

Column: 30
Error code: E203
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been dropped. Show all issues
```
whitespace before ':'

Column: 30
Error code: E203
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 13
Error code: W503
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 13
Error code: W503
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 13
Error code: W503
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 13
Error code: W503
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 21
Error code: W503
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 21
Error code: W503
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 21
Error code: W503
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 17
Error code: W503
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 17
Error code: W503
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 13
Error code: W503
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 13
Error code: W503
```
reviewboard/diffviewer/diffutils.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 17
Error code: W503
```
reviewboard/diffviewer/diffutils.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line too long (80 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/diffviewer/diffutils.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 17
Error code: W503
```
reviewboard/diffviewer/diffutils.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 17
Error code: W503
```
reviewboard/diffviewer/diffutils.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 21
Error code: W503
```
reviewboard/diffviewer/diffutils.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 25
Error code: W503
```
reviewboard/diffviewer/diffutils.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 25
Error code: W503
```
reviewboard/diffviewer/diffutils.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 25
Error code: W503
```
reviewboard/diffviewer/diffutils.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line break before binary operator

Column: 17
Error code: W503
```

Fix it!

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.

reviewboard/diffviewer/chunk_generator.py (Diff revision 1)

An issue was opened. Show all issues

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.

reviewboard/diffviewer/chunk_generator.py (Diff revision 1)

An issue was opened. Show all issues

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.

reviewboard/diffviewer/chunk_generator.py (Diff revision 1)

An issue was opened. Show all issues

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.

reviewboard/diffviewer/chunk_generator.py (Diff revision 1)

An issue was opened. Show all issues

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.

reviewboard/diffviewer/chunk_generator.py (Diff revision 1)

An issue was opened. Show all issues

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.

maubin Oct. 27, 2023, 9:30 a.m.
```
Agreed.
```

reviewboard/diffviewer/chunk_generator.py (Diff revision 1)

An issue was opened. Show all issues

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 if statements. It's a choice for sure, but noe one I'm on board with.

reviewboard/diffviewer/chunk_generator.py (Diff revision 1)

An issue was opened. Show all issues

Huh, this seems like a strange thing for Black to insist upon. I don't think it buys us anything at all.

reviewboard/diffviewer/chunk_generator.py (Diff revision 1)

An issue was opened. Show all issues

This is similar to the yield self.new_chunk up 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.

Review Board 7.0 alpha 0 (dev)

Discussion: auto-formatting (diffviewer).

Information

Reviewers

flake8

Status: Discarded