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 <j...@abou-samra.fr
<mailto:j...@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