Policy for automated code reformatting?

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Policy for automated code reformatting?

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?

Cheers,
Jean


Reply | Threaded
Open this post in threaded view
|

Re: Policy for automated code reformatting?

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.

Jonas

signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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 (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Policy for automated code reformatting?

Jean ABOU SAMRA

Le 05/09/2020 à 15:08, Jonas Hahnfeld a écrit :

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

Well, I think Han-Wen would like this for convenience (Han-Wen, am
I right?).Personally I don't like automatically formatting code that
I write, but I can understand that those who write code in different
languages
and styles make their lives simpler by running an automatic
formatter on the files affected by their patch.

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

Sure.

Jean

> Jonas