Re: Python coding style

[Jean Abou Samra]

Hi Carl,

Thanks for your reply.

Le 01/07/2020 à 17:05, Carl Sorensen a écrit :
> Hi Jean,
>
> On Wed, Jul 1, 2020 at 6:03 AM Jean Abou Samra <jean@abou-samra.fr 
> <mailto:jean@abou-samra.fr>> wrote:
>
>     Hi everybody,
>
>     There is some discussion in !212 about coding style inside our Python
>     scripts.
>
>     The Contributor's Guide (10.5.1) clearly states that "Python code
>     should
>     use PEP 8". So, I'd like to be sure everyone agrees on the
>     following points
>     which are part of applying this PEP:
>
>     - Remove spaces before the opening parenthesis in function calls and
>     definitions,
>     e.g., f(x) instead of f (x).
>
>
> I believe we should definitely follow PEP here.  And ideally we should 
> have a code formatter that does this for us, so we don't rely on 
> people remembering that PEP is different from GNU C standarads.

Indeed, that was also the subject of a discussion in !190 
(https://gitlab.com/lilypond/lilypond/-/merge_requests/190). Since then, 
I looked a very bit into available Python linters and found Pylint which 
looks great. It could be an ultimate step to use it in CI. (So Han-Wen, 
unlike when using Mypy, you don't need to annotate the code you write in 
order to check for bare typos.)

Running pylint on scripts/abc2ly.py outputs 1687 lines of tips. Excerpts:

scripts/abc2ly.py:159:0: W1401: Anomalous backslash in string: '\+'. 
String constant might be missing an r prefix. 
(anomalous-backslash-in-string)
scripts/abc2ly.py:132:6: C0326: Exactly one space required around assignment
DIGITS='0123456789'
      ^ (bad-whitespace)
scripts/abc2ly.py:137:10: C0326: No space allowed before bracket
def error (msg):
          ^ (bad-whitespace)
scripts/abc2ly.py:164:0: W0301: Unnecessary semicolon 
(unnecessary-semicolon)
scripts/abc2ly.py:167:0: W0311: Bad indentation. Found 6 spaces, 
expected 8 (bad-indentation)
scripts/abc2ly.py:218:21: C0326: Exactly one space required after comma
def dump_header (outf,hdr):
                     ^ (bad-whitespace)
scripts/abc2ly.py:228:0: C0325: Unnecessary parens after 'if' keyword 
(superfluous-parens)
scripts/abc2ly.py:650:0: C0325: Unnecessary parens after 'return' 
keyword (superfluous-parens)
scripts/abc2ly.py:654:15: C0326: No space allowed around bracket
    a = re.sub ( '_', ' _ ', a)        # _ to ' _ '
               ^ (bad-whitespace)
scripts/abc2ly.py:655:0: C0301: Line too long (105/100) (line-too-long)
scripts/abc2ly.py:1237:20: W0622: Redefining built-in 'str' 
(redefined-builtin)

In the end, it says "Your code has been rated at 0.89/10.". (For 
disclosure, I'm rather proud that in the branch where my current work on 
abc2ly takes place, this bumps to 3.36/10.)

>     - Use `string` instead of `str` as an identifier − `str` being a
>     built-in type
>     already. Also common is `s`, but the Contributor's Guide also says
>     (10.5.4)
>     that "Variable names should be complete words, rather than
>     abbreviations."
>     (which is basically concerned with C++, but that particular rule
>     is, in my
>     opinion, valid for Python too). In particular, this applies to
>     python/convertrules.py.
>
>
> I can agree that it is wrong to use 'str', since 'str' is a built in 
> type.  In my opinion, for convertrules.py, we shoud just use 's', not 
> 'string'. Even if we use the complete word 'string', there is no 
> special meaning in it (it's not a special string, it's just a generic 
> string).  I believe it's analogous to using 'i' for an index in a c++ 
> loop.

Why not. I agree that `string` would be somehow superfluous for 
convertrules.py at least.

>     - Use the standard naming conventions, especially CamelCase for class
>     names (the
>     current style being a mixture of CamelCase and sometimes
>     First_word_capitalized_with_underscores). Write module-level
>     constants in
>     UPPERCASE_WITH_UNDERSCORES. Likewise, change things like
>        class error (Exception): pass
>     to
>        class MIDIConversionError(Exception):
>            pass
>
>     In CamelCase names that contain acronyms, capitalize all letters
>     of the
>     acronym
>     (thus, the previous example doesn't read 'MidiConversionError').
>
>
> I think we should follow the standard here.
>
>     There are many specific changes that could be done to improve
>     style and
>     readability
>     (such as replacing `ls = list(something); ls.sort()` with `ls =
>     sorted(something)`,
>     etc.), but the above could be made in general commits fixing all
>     Python
>     files.
>
>
> I would be supportive of changes to improve style and readability, but 
> might question if it's worth the time. However, I'd support anybody 
> who wants to scratch that itch.
>
> Making things compatible with PEP8 is certainly a good idea.


What I plan is to apply changes that can be done in a semi-automatical 
fashion to all Python files (in order to facilitate review). For other 
types of improvement, I don't intend to do anything apart from cleaning 
up a particular file if I need to touch it.

Also, to be honest, my time clearly isn't as valuable for the project as 
that of a LilyPond developer, so I can spend it on small tasks like this 
which are within my reach.

Cheers,
Jean

> Thanks,
>
> Carl