Re: Proposal: format GDB Python files with black

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Simon Marchi <simon.marchi@polymtl.ca> [2021-04-26 13:25:37 -0400]:

> On 2021-04-26 12:21 p.m., Andrew Burgess wrote:
> > * Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-04-26 11:55:21 -0400]:
> > 
> >> Hi all,
> >>
> >> A few people I talked to about this and I have good experience with the
> >> tool black to auto-format Python code.  It is simple to use, fast and
> >> reliable (if it detects that it changed the meaning of the program by
> >> mistake, internal-errors out).  I don't think I need to spell out the
> >> advantage of using such a tool, but just in case: it removes all the
> >> overhead of thinking about formatting when writing or reviewing code.
> >>
> >> My suggestion would be to format our code with black all at once.  The
> >> typical counter-argument to this is "it will be harder to use
> >> git-blame".  I don't think this is true, because you need to be able to
> >> skip over commits anyway, and it's trivial to skip over a commit when
> >> git-blaming using an interactive tool.  But it's also possible to tell
> >> git to ignore certain commits when git-blaming [2], so we can do that.
> >>
> >> I think the output of black is quite readable.  When it does weird
> >> stuff, it's generally because the lines / expressions are two long
> >> anyway, and deserve to be split in multiple lines / expressions.  Here's
> >> a branch that shows how it would look like:
> > 
> > In general, the idea of auto-formatting the code sounds great, but I
> > would like to understand more about how the process would work.
> > 
> > Right now, if I make a change I edit a file, git add, git commit, and
> > finally, git push.  At what point does the auto-formatting take place?
> > Do I need to manually run the tool after editing the file?  Is it
> > done, well, automatically at some point?  And if it is done
> > automatically, then at what point in the process does this happen, and
> > how does this relate to code review?
> 
> Those are good questions.
> 
> At the simplest level, the individual making changes is responsible for
> the tool at any time they want, as long as it's before sending the patch
> and / or pushing to master.  So it would be pretty much like today,
> where we are responsible for formatting the source code correctly,
> except that it would now be tool-assisted instead of done mentally.
> 
> How to run the tool is left to each individual:
> 
> 1. By hand on the command line
> 2. From the editor [1]
> 3. Using a git-commit hook

So without going more sophisticated as you describe below we'd still
be relying on reviewers to either (with enough experience) "just know"
that the code is black formatted, or apply the patch, run black, and
see if the formatting changes.

Then of course, there's the questions about what happens when
different users are running different versions of black - assuming
different versions might format things slightly differently.

Another concern I'd have is that unrelated "formatting" changes might
creep in.

So, I edit a .py file and either don't run black (naughty), or use
version X of black.

Much later, I edit a different part of the same file, now either I run
black, or run version Y of black.  My original edit is reformatted
slightly differently.

My assumption is that this change (the second patch) should be
rejected at review, as the reformatting change (of the earlier code)
is unrelated to the work of the second patch.  But I suspect some folk
might find it frustrating, if on one had we say run black, but on the
other hand we say you need to exclude unrelated chunks.

I think, in the heterogeneous environments we all develop in,
situations like the above will crop up, so it would be important to
know what the expectations would be in such a case.

> 
> If we want to be a bit more sophisticated, we could check-in a git
> commit hook that runs black (if available) to check your formatting as
> you are committing, if your commit includes changes to Python files.
> That doesn't prevents formatting errors from slipping in the repo, but
> it's already a good start (and formatting errors slipping in the repo
> are not the end of the world).

Again, this sort of thing works great in a corporate environment where
you can guarantee that everyone is (or has no excuse not to be)
running the exact same version of every tool.

My concern would be that such a strategy would invite unintended
changes to creep in.

> 
> We don't really have CI right now (especially pre-merge testing), but if
> we had, checking the formatting of Python files could be a validation
> step.

Agreed.

> We do however, have scripts that run server side on push, so it would be
> possible to run black server-side to reject commits that would introduce
> formatting errors.

I guess this would solve a lot of the problems I'm concerned about.
Once we've "corrected" the formatting of all .py code then nobody
could merge bad code.

Thanks for all the info,

Andrew