Discussion: auto-formatting (diffviewer).

Review Request #13380 — Created Oct. 26, 2023 and discarded

Information

Review Board
master

Reviewers

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

 
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
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 …

chipx86chipx86

This gives us less room per line to work with localized strings. It's not the worst thing in the world, …

chipx86chipx86

I'm confused as to why it chose this presentation. There are other examples like this, but it seems inconsistent. My …

chipx86chipx86

I don't mind this format when we're doing type hints, and think it provides a lot of value there. In …

chipx86chipx86

line too long (80 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

I'm fully against this. The multi-line format is far more readable than what Black is choosing to produce here, and …

chipx86chipx86

This particular conditional was not great, and I understand what Black is trying to do here with the whole "the …

chipx86chipx86

line break before binary operator Column: 13 Error code: W503

reviewbotreviewbot

line break before binary operator Column: 13 Error code: W503

reviewbotreviewbot

line break before binary operator Column: 13 Error code: W503

reviewbotreviewbot

line break before binary operator Column: 13 Error code: W503

reviewbotreviewbot

line break before binary operator Column: 13 Error code: W503

reviewbotreviewbot

line break before binary operator Column: 13 Error code: W503

reviewbotreviewbot

line break before binary operator Column: 17 Error code: W503

reviewbotreviewbot

whitespace before ':' Column: 30 Error code: E203

reviewbotreviewbot

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

chipx86chipx86

whitespace before ':' Column: 30 Error code: E203

reviewbotreviewbot

line break before binary operator Column: 13 Error code: W503

reviewbotreviewbot

line break before binary operator Column: 13 Error code: W503

reviewbotreviewbot

This is similar to the yield self.new_chunk up above. Short of using keyword arguments and multiple lines, I don't think …

chipx86chipx86

line break before binary operator Column: 13 Error code: W503

reviewbotreviewbot

line break before binary operator Column: 13 Error code: W503

reviewbotreviewbot

line break before binary operator Column: 21 Error code: W503

reviewbotreviewbot

line break before binary operator Column: 21 Error code: W503

reviewbotreviewbot

line break before binary operator Column: 21 Error code: W503

reviewbotreviewbot

line break before binary operator Column: 17 Error code: W503

reviewbotreviewbot

line break before binary operator Column: 17 Error code: W503

reviewbotreviewbot

line break before binary operator Column: 13 Error code: W503

reviewbotreviewbot

line break before binary operator Column: 13 Error code: W503

reviewbotreviewbot

line break before binary operator Column: 17 Error code: W503

reviewbotreviewbot

line too long (80 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line break before binary operator Column: 17 Error code: W503

reviewbotreviewbot

line break before binary operator Column: 17 Error code: W503

reviewbotreviewbot

line break before binary operator Column: 21 Error code: W503

reviewbotreviewbot

line break before binary operator Column: 25 Error code: W503

reviewbotreviewbot

line break before binary operator Column: 25 Error code: W503

reviewbotreviewbot

line break before binary operator Column: 25 Error code: W503

reviewbotreviewbot

line break before binary operator Column: 17 Error code: W503

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

chipx86
  1. 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.

  2. reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
     
     
     
     
    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.

  3. reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
     
     
     
     
     
    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.

  4. reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
     
     
     
     
    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.

  5. reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
     
     
     
     
    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.

  6. reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
     
     
     
     
     
    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.

  7. reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
     
     
     
     
     
    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.

  8. 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.

  9. reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
     
     
     
     
    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.

  10. 
      
david
Review request changed
Status:
Discarded