Re: Policy for automated code reformatting?

[Jonas Hahnfeld]

Am Samstag, den 05.09.2020, 15:03 +0200 schrieb Jonas Hahnfeld:
> Am Samstag, den 05.09.2020, 14:54 +0200 schrieb Jean Abou Samra:
> > Hi,
> > 
> > We talked several times about automated code reformatting these times,
> > the last instance being 
> > https://gitlab.com/lilypond/lilypond/-/merge_requests/375.
> > There is general agreement (I think) that we need some way of 
> > maintaining the quality
> > of Python code in particular. I feel it'd be hard to impose a certain 
> > formatting tool
> > to be run on each Merge Request. On the other hand, automated 
> > reformattings, if not
> > practised by everyone, affect other parts of the same file, which 
> > doesn't facilitate
> > review, and there is the git blame issue. I propose a compromise here, 
> > with a policy to
> > harmonize things a bit:
> > 
> >     New code is required to comply with the project's coding style; 
> > older code *can* be
> >     updated to follow this style. Developers *may* use ``fixcc``, 
> > ``fixscm`` and ``autopep8``
> >     to reformat code automatically as part of a Merge Request. In doing 
> > so, developers *may*
> >     reformat other parts of the same file that would otherwise be left 
> > untouched by their
> >     patch. In this case, the reformatting *must* be done in a separate 
> > commit. Under normal
> >     circumstances, reformattings *should* be limited to single files.
> > 
> > Rationale:
> > - Those who so like can use these tools freely on the code they write.
> > - Reviewers can review commits, so they may skip the reformatting.
> > - There does not need to be a countdown just for the reformatting.
> > - Don't hamper the usability of git blame too much, only reformat code 
> > when it
> >    is modified.
> > 
> > What do you think?
> 
> I think there's a policy for C++ formatting that we just run fixcc.py
> from time to time, and the same happens for fixscm.py. We should do the
> same for Python with autopep8. I certainly don't get the benefit of
> introducing formatting changes into merge requests.

Nah, that was ambiguous: I don't see the benefit of introducing
*automated* formatting changes in *unrelated* parts of a file into
reviewed merge requests. Even in a separate commit, that still comes
with disadvantages for the bot trying to understand if there was a
change between two versions of a merge request.
Warning about modified parts that don't follow the style might be a
good addition. For everything else, just running the tools once in a
while should do the job.

Jonas

signature.asc
Description: This is a digitally signed message part