[th/reformat-black] black: reformat all code with python black

[thom311]

• add a .github test for checking the formatting.

• for python scripts without extension, we must list the filenames in pyproject.toml.

• the version of black used for reformatting was 24.4.2. But 22.12.0 produces the same output.

[bn222]

Can we increase the max line length to avoid breaking lines?

[thom311]

That seems a bad idea.

If the line is so long that it breaks, then the wrapping of the line happens to increase readability. Stuffing more things on one line does not improve it. Also, often a line is just long (and needs wrapping) for good reasons, there is nothing wrong with the line break, and there is nothing to avoid. The long line is (often) fine, and the wrapping is fine.

Black calls itself uncompromising ([1]), configuring it is against that idea. The benefit is that all black formatted projects look the same and you don't have to think about project specific settings.

Also, the amounts of discussion and expertise that went into black and PEP8 (which black mostly implements) are beyond any single human. It's not our place to second guess that.

Also -- as any style question, this is entirely my opinion -- CDA with those long lines shows very clearly that this is a bad idea.

[thom311]

Automated code formatting is about having an automated, deterministic/reproducible way for formatting the code, it's the least about what each of us personally likes.

---

Having said that, I also don't particularly like

    minicom_watch = threading.Thread(
        target=capture_minicom, args=(args, stop_event, output)
    )

If you add a trailing comma, black will format this as

    minicom_watch = threading.Thread(
        target=capture_minicom, 
        args=(args, stop_event, output),
    )

or even

    minicom_watch = threading.Thread(
        target=capture_minicom, 
        args=(
            args,
            stop_event,
            output,
        ),
    )

I like the latter two.

All of the above looks better to me than

minicom_watch = threading.Thread(target=capture_minicom, args=(args, stop_event, output))

---

So, while it's not about what I (or anybody) personally prefers, I would rather stick with the default that black provides, where experts decided that this is how python should look like. Incidentally, that is what I also prefer (which is probably due to getting used to it).

[bn222]

I agree with the intent of auto-formatting: it avoids discussions. However, an important thing within this context for me is that if a line gets broken up by line-breaks, it tells me that the code needs work. Auto-formatting to me is a crude way to make code conform to some standard. I don't care about spaces after or before :, I don't care about " or ' as long as it's consistent.

In this case, I would rewrite it as:

target = lambda: capture_minicom(args, stop_event, output)
minicom_watch = threading.Thread(target=target)

The added benefit is that it's more a functional programming style. In many cases, where a formatter breaks up the lines, it has been a way for me to detect that I need to work on the code. My approach in these cases is to patch up the code first, so that when I apply the formatter, no line breaks are introduced. That takes effort, but the end-result is more modular code. That's also one of the reasons why I haven't added a formatter in the first place to the repo.

This is all subjective, and it's based on my experience when I need to maintain and debug code. For me, the order of preference has been:

1. Short lines
2. Long lines
3. Newlines to break up long lines

We might disagree on 2. having a preference over 3., but do we agree that 1. sits at the top?

[thom311]

However, an important thing within this context for me is that if a line gets broken up by line-breaks, it tells me that the code needs work.

I don't agree with this statement as an absolute. If the line is too long, it should to be wrapped. If there is still a problem, it should to be refactored (but a long line is usually not the solution).

---

So why are lines now breaking? You previously wrote

    minicom_watch = threading.Thread(target=capture_minicom, args=(args, stop_event, output))

and you (presumably) thought this was fine. Based on that example, you seem to think that an acceptable maximum line length is at least 93 characters. Which is a fine opinion.

Except, black uses a maximum line length of 88 characters. It's explained here why that is chosen. This was chosen by experts in the area of wrapping lines in Python code. Note that python-black mostly implements PEP8. PEP8 recommends even a shorter maximum line length of 79. I think those experts had their good reasons, and there is no need for us to ponder problems that have reached a general consensus (python-black implements the consensus). Accepting it means that other black-formatted projects look the same!

There wasn't a problem with the code with the long line. There is also no problem after black wrapping it. Reformatting with black however solves the problem to get a uniform, automated, deterministic style.

In many cases, where a formatter breaks up the lines, it has been a way for me to detect that I need to work on the code

If the formatter doesn't run, you don't see where lines would be broken.

[bn222]

Yes, I initially wrote the line:

minicom_watch = threading.Thread(target=capture_minicom, args=(args, stop_event, output))

I'm typically using a maximum line length of 120. I'm open to 80 or 88, as long as it doesn't introduce unnecessary line breaks. Enabling the formatter highlights this line because it wants to break it up. To avoid this, I propose rewriting lines in the code-base like the above to (still PEP8 compliant):

target = lambda: capture_minicom(args, stop_event, output)
minicom_watch = threading.Thread(target=target)

Before running the formatter, I think we should refactor the whole code base to make it more readable and maintainable. Someone should manually review and reformat the code to better convey its intent, breaking up long lines into multiple lines in a way that makes sense. I don't mean that the code should be blindly broken up. Rather, when breaking up the lines, someone doing the refactoring should think about it a bit more. This approach ensures code quality improves, rather than forcefully breaking up lines to meet a character limit. Once we apply a PR that simply "runs black", we can't undo it anymore or easily see where the code needs work. If we do the refactoring first, we can avoid unnecessary breaks and make the code more readable. This is also the main reason I don't add formatting rules to a project until after I have refactored the code so that the formatter makes minor changes.

[thom311]

OK. Closing this PR thus, as it only does simply run black.

[bn222]

Please do the refactoring, and then enable black on top (reopen this PR afterwards and rerun black on top)

[thom311]

@bn222 I am sorry, but I am not quite agreeable on this way forward.

The line

    minicom_watch = threading.Thread(target=capture_minicom, args=(args, stop_event, output))

was fine before. And afterwards

    minicom_watch = threading.Thread(
        target=capture_minicom, args=(args, stop_event, output)
    )

is still just as fine. Actually, in my personal opinion the latter is even slightly better to read, because less things are cramped in one line. So, subjectively, the reformatting makes the code a tiny bit better, but it doesn't matter either way (what matters it to move over to an automated formatter and follow the style that it defines).

Wrapping lines is not a problem or a code-smell in general. Sometimes it may be, and one might ponder whether it's worth to improve something. What we see in this patch, does not show any problem that requires manual intervention.

---

Obviously, you disagree with that. Please proceed in any way you think best (including doing nothing, never doing python-black, using some other line-length, manually adjusting lines before reformatting). I explained, why in my option those things are not the right thing to do. I'd rather not do that.