Run formatting tools

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

Run formatting tools

Jonas Hahnfeld
Hi all,

with the development slowing down, I'd like to run the formatting tools
on LilyPond's source code (fixcc.py, fixscm.py, autopep8). I've pushed
this as https://gitlab.com/lilypond/lilypond/-/merge_requests/422 and
the changes for fixcc.py and fixscm.py make sense to me.
What puzzles me a bit is the magnitude of changes from autopep8 - I
thought this tool was run in August? Maybe I'm missing something; FWIW
I used the invocation "autopep8 -ia --ignore=E402" as documented in CG.

Thoughts, objections? Should I exclude running autopep8?

Jonas


P.S.: Dan's work on voltas rebases cleanly on the formatting changes;
most files touched in !386 needed no formatting changes and the few
that did only have changes in different parts of the file that git can
handle automatically.

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

Re: Run formatting tools

Werner LEMBERG

> What puzzles me a bit is the magnitude of changes from autopep8 - I
> thought this tool was run in August? Maybe I'm missing something;
> FWIW I used the invocation "autopep8 -ia --ignore=E402" as
> documented in CG.
>
> Thoughts, objections? Should I exclude running autopep8?

I was told from a Python professional developer that the best tool for
formatting python code be `black`.  Running

  black -l 78 ...

gives indeed very nice formatting (not tested with lilypond stuff,
though).


    Werner

Reply | Threaded
Open this post in threaded view
|

Re: Run formatting tools

Jonas Hahnfeld
Am Freitag, den 25.09.2020, 09:31 +0200 schrieb Werner LEMBERG:

> > What puzzles me a bit is the magnitude of changes from autopep8 - I
> > thought this tool was run in August? Maybe I'm missing something;
> > FWIW I used the invocation "autopep8 -ia --ignore=E402" as
> > documented in CG.
> >
> > Thoughts, objections? Should I exclude running autopep8?
>
> I was told from a Python professional developer that the best tool for
> formatting python code be `black`.  Running
>
>   black -l 78 ...
>
> gives indeed very nice formatting (not tested with lilypond stuff,
> though).
Hm, I thought we agreed on using autopep8 more than two months ago? I'm
not very familiar with either tool but I'm against switching back and
forth multiple times, that'll just cause churn in the code.

Jonas

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

Re: Run formatting tools

Han-Wen Nienhuys-3
On Fri, Sep 25, 2020 at 9:36 AM Jonas Hahnfeld <[hidden email]> wrote:

> Am Freitag, den 25.09.2020, 09:31 +0200 schrieb Werner LEMBERG:
> > > What puzzles me a bit is the magnitude of changes from autopep8 - I
> > > thought this tool was run in August? Maybe I'm missing something;
> > > FWIW I used the invocation "autopep8 -ia --ignore=E402" as
> > > documented in CG.
> > >
> > > Thoughts, objections? Should I exclude running autopep8?
> >
> > I was told from a Python professional developer that the best tool for
> > formatting python code be `black`.  Running
> >
> >   black -l 78 ...
> >
> > gives indeed very nice formatting (not tested with lilypond stuff,
> > though).
>
> Hm, I thought we agreed on using autopep8 more than two months ago? I'm
> not very familiar with either tool but I'm against switching back and
> forth multiple times, that'll just cause churn in the code.
>

IIRC I think the option used was -aa (2x -a = aggressive.)

--
Han-Wen Nienhuys - [hidden email] - http://www.xs4all.nl/~hanwen
Reply | Threaded
Open this post in threaded view
|

Re: Run formatting tools

Jean ABOU SAMRA
Le 25/09/2020 à 12:58, Han-Wen Nienhuys a écrit :

>
>
> On Fri, Sep 25, 2020 at 9:36 AM Jonas Hahnfeld <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Am Freitag, den 25.09.2020, 09:31 +0200 schrieb Werner LEMBERG:
>     > > What puzzles me a bit is the magnitude of changes from
>     autopep8 - I
>     > > thought this tool was run in August? Maybe I'm missing something;
>     > > FWIW I used the invocation "autopep8 -ia --ignore=E402" as
>     > > documented in CG.
>     > >
>     > > Thoughts, objections? Should I exclude running autopep8?
>     >
>     > I was told from a Python professional developer that the best
>     tool for
>     > formatting python code be `black`.  Running
>     >
>     >   black -l 78 ...
>     >
>     > gives indeed very nice formatting (not tested with lilypond stuff,
>     > though).
>
>     Hm, I thought we agreed on using autopep8 more than two months
>     ago? I'm
>     not very familiar with either tool but I'm against switching back and
>     forth multiple times, that'll just cause churn in the code.
>
>
> IIRC I think the option used was -aa (2x -a = aggressive.)

It's the opposite, actually. I ran autopep8 wit the default level of
aggressivity (no -a option), to be sure not to cause any regressions. Since
then the right fixes to exclude were found and we put something different
into the CG.

Black may be a good option. A good reason for using autopep8 back in the
time was its fix for invalid escape sequences. Which turned out to be
catastrophic. Since they are all fixed by now, and the interpreter will
emit warnings during bytecode compilation if any new are added, I don't
see any reason not to switch to Black. It's much less configurable, with
just good defaults, and has become a standard tool nowadays as far as I
can see (at least, it's in the PSF organisation on GitHub).

You could put this in a pyproject.toml file:

[tool.black]
line-length  =  78

Then the command would be just ``black .``

Thanks,
Jean

PS: After a quick look, I indeed prefer Black's reformatting over
autopep8's.

Reply | Threaded
Open this post in threaded view
|

Re: Run formatting tools

Jonas Hahnfeld
Am Freitag, den 25.09.2020, 14:13 +0200 schrieb Jean Abou Samra:

> Le 25/09/2020 à 12:58, Han-Wen Nienhuys a écrit :
> > On Fri, Sep 25, 2020 at 9:36 AM Jonas Hahnfeld <[hidden email]> wrote:
> > > Am Freitag, den 25.09.2020, 09:31 +0200 schrieb Werner LEMBERG:
> > > > > What puzzles me a bit is the magnitude of changes from autopep8 - I
> > > > > thought this tool was run in August? Maybe I'm missing something;
> > > > > FWIW I used the invocation "autopep8 -ia --ignore=E402" as
> > > > > documented in CG.
> > > > >
> > > > > Thoughts, objections? Should I exclude running autopep8?
> > > >
> > > > I was told from a Python professional developer that the best tool for
> > > > formatting python code be `black`.  Running
> > > >
> > > >   black -l 78 ...
> > > >
> > > > gives indeed very nice formatting (not tested with lilypond stuff,
> > > > though).
> > >
> > > Hm, I thought we agreed on using autopep8 more than two months ago? I'm
> > > not very familiar with either tool but I'm against switching back and
> > > forth multiple times, that'll just cause churn in the code.
> > >
> >
> > IIRC I think the option used was -aa (2x -a = aggressive.)
>
> It's the opposite, actually. I ran autopep8 wit the default level of
> aggressivity (no -a option), to be sure not to cause any regressions. Since
> then the right fixes to exclude were found and we put something different
> into the CG.
>
> Black may be a good option. A good reason for using autopep8 back in the
> time was its fix for invalid escape sequences. Which turned out to be
> catastrophic. Since they are all fixed by now, and the interpreter will
> emit warnings during bytecode compilation if any new are added, I don't
> see any reason not to switch to Black. It's much less configurable, with
> just good defaults, and has become a standard tool nowadays as far as I
> can see (at least, it's in the PSF organisation on GitHub).
>
> You could put this in a pyproject.toml file:
>
> [tool.black]
> line-length=78
>
> Then the command would be just ``black .``
Not sure why 78 characters? PEP 8 says 79 characters for code or at
most 72 for doc-strings.

Anyway, running black on all Python files gives the following error:

    error: cannot format /code/LilyPond/src/scripts/auxiliar/translations-status.py: INTERNAL ERROR: Black produced different code on the second pass of the formatter.

Also, it seems to change every occurrence of single quotes to double
quotes unless there are already double quotes. My impression is that
single quotes are used more widespread, and the diff is massive at
56 files changed, 10819 insertions(+), 7704 deletions(-)

I don't care enough of the Python formatting for now so I've removed
that commit from the mentioned MR. The obvious caveat is that we must
not run such large-scale formatting if we finally create a stable
branch (not discussed here), or apply the same run for the branch as
well.

Jonas

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

Re: Run formatting tools

Jean ABOU SAMRA

Le 25/09/2020 à 15:48, Jonas Hahnfeld a écrit :
>> You could put this in a pyproject.toml file:
>>
>> [tool.black]
>> line-length=78
>>
>> Then the command would be just ``black .``
> Not sure why 78 characters? PEP 8 says 79 characters for code or at
> most 72 for doc-strings.

I don't know, just took this figure from Werner.

> Anyway, running black on all Python files gives the following error:
>
>      error: cannot format /code/LilyPond/src/scripts/auxiliar/translations-status.py: INTERNAL ERROR: Black produced different code on the second pass of the formatter.
I can't reproduce this. What is your version of Black? (Mine is 19.3b0.)
What line length did you use?
> Also, it seems to change every occurrence of single quotes to double
> quotes unless there are already double quotes. My impression is that
> single quotes are used more widespread, and the diff is massive at
> 56 files changed, 10819 insertions(+), 7704 deletions(-)
https://github.com/psf/black#testimonials

Dusty Phillips, writer
<https://smile.amazon.com/s/ref=nb_sb_noss?url=search-alias%3Daps&field-keywords=dusty+phillips>:

    /Black/is opinionated so you don't have to be.

That's both the advantage and the drawback.

Personally I don't find the diff to be a huge problem
since the autopep8 reformatting already modified the blame.

> I don't care enough of the Python formatting for now so I've removed
> that commit from the mentioned MR. The obvious caveat is that we must
> not run such large-scale formatting if we finally create a stable
> branch (not discussed here), or apply the same run for the branch as
> well.
Yeah, Python should be tackled in another MR if at all. Thanks
for raising this.

Best,
Jean

Reply | Threaded
Open this post in threaded view
|

Re: Run formatting tools

Jonas Hahnfeld
Am Freitag, den 25.09.2020, 16:01 +0200 schrieb Jean Abou Samra:
> Le 25/09/2020 à 15:48, Jonas Hahnfeld a écrit :
> > Anyway, running black on all Python files gives the following error:
> >
> >     error: cannot format /code/LilyPond/src/scripts/auxiliar/translations-status.py: INTERNAL ERROR: Black produced different code on the second pass of the formatter.
>  I can't reproduce this. What is your version of Black? (Mine is 19.3b0.) What line length did you use?

The latest, 20.8b1. Happens with both the default and -l 78.

> > Also, it seems to change every occurrence of single quotes to double
> > quotes unless there are already double quotes. My impression is that
> > single quotes are used more widespread, and the diff is massive at
> > 56 files changed, 10819 insertions(+), 7704 deletions(-)
>  https://github.com/psf/black#testimonials
> Dusty Phillips, writer:
>
> > Black is opinionated so you don't have to be.
> >
>
> That's both the advantage and the drawback.
>
> Personally I don't find the diff to be a huge problem
> since the autopep8 reformatting already modified the blame.
If it changes around about every quoted string, things can go wrong
very quickly because of the weird escaping rules. We had a similar
issue with the import reordering and I personally don't want to track
down such issue once more.

Jonas

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

Re: Run formatting tools

Jean ABOU SAMRA
Le 25/09/2020 à 16:39, Jonas Hahnfeld a écrit :

> Am Freitag, den 25.09.2020, 16:01 +0200 schrieb Jean Abou Samra:
>> Le 25/09/2020 à 15:48, Jonas Hahnfeld a écrit :
>>> Anyway, running black on all Python files gives the following error:
>>>
>>>      error: cannot format /code/LilyPond/src/scripts/auxiliar/translations-status.py: INTERNAL ERROR: Black produced different code on the second pass of the formatter.
>>   I can't reproduce this. What is your version of Black? (Mine is 19.3b0.) What line length did you use?
>>
>> The latest, 20.8b1. Happens with both the default and -l 78.

Seems to be a known, recently introduced bug in Black.It's easy
to work around it using --fast.

>>> Also, it seems to change every occurrence of single quotes to double
>>> quotes unless there are already double quotes. My impression is that
>>> single quotes are used more widespread, and the diff is massive at
>>> 56 files changed, 10819 insertions(+), 7704 deletions(-)
>>   https://github.com/psf/black#testimonials
>> Dusty Phillips, writer:
>>
>>> Black is opinionated so you don't have to be.
>>>
>> That's both the advantage and the drawback.
>>
>> Personally I don't find the diff to be a huge problem
>> since the autopep8 reformatting already modified the blame.
> If it changes around about every quoted string, things can go wrong
> very quickly because of the weird escaping rules. We had a similar
> issue with the import reordering and I personally don't want to track
> down such issue once more.

To be clear, I was not asking you to take any action.

Best,
Jean


Reply | Threaded
Open this post in threaded view
|

Re: Run formatting tools

Jonas Hahnfeld
Am Freitag, den 25.09.2020, 21:14 +0200 schrieb Jean Abou Samra:

> Le 25/09/2020 à 16:39, Jonas Hahnfeld a écrit :
>
> > Am Freitag, den 25.09.2020, 16:01 +0200 schrieb Jean Abou Samra:
> > > Le 25/09/2020 à 15:48, Jonas Hahnfeld a écrit :
> > > > Anyway, running black on all Python files gives the following error:
> > > >
> > > >      error: cannot format /code/LilyPond/src/scripts/auxiliar/translations-status.py: INTERNAL ERROR: Black produced different code on the second pass of the formatter.
> > >
> > >   I can't reproduce this. What is your version of Black? (Mine is 19.3b0.) What line length did you use?
> > >
> > > The latest, 20.8b1. Happens with both the default and -l 78.
>
> Seems to be a known, recently introduced bug in Black.It's easy
> to work around it using --fast.
This will just skip the sanity check that future runs keep the
formatting. I don't consider this a valid workaround because we *will*
run the tool again later on.

Jonas

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

Re: Run formatting tools

Jean ABOU SAMRA

Le 26/09/2020 à 10:26, Jonas Hahnfeld a écrit :

> Am Freitag, den 25.09.2020, 21:14 +0200 schrieb Jean Abou Samra:
>> Le 25/09/2020 à 16:39, Jonas Hahnfeld a écrit :
>>
>>> Am Freitag, den 25.09.2020, 16:01 +0200 schrieb Jean Abou Samra:
>>>> Le 25/09/2020 à 15:48, Jonas Hahnfeld a écrit :
>>>>> Anyway, running black on all Python files gives the following error:
>>>>>
>>>>>       error: cannot format /code/LilyPond/src/scripts/auxiliar/translations-status.py: INTERNAL ERROR: Black produced different code on the second pass of the formatter.
>>>>    I can't reproduce this. What is your version of Black? (Mine is 19.3b0.) What line length did you use?
>>>>
>>>> The latest, 20.8b1. Happens with both the default and -l 78.
>> Seems to be a known, recently introduced bug in Black.It's easy
>> to work around it using --fast.
> This will just skip the sanity check that future runs keep the
> formatting. I don't consider this a valid workaround because we *will*
> run the tool again later on.

Actually, subsequent runs do keep the same formatting, but I don't
understand why.

Jean