Re: Python coding style

[Jean Abou Samra]

Hi,

Le 02/07/2020 à 09:05, Han-Wen Nienhuys a écrit :

You can also do a global cleanup. We have done this in the past, but
it has the disadvantage that it makes history (eg. git-blame) harder
to read.



So, I did the global cleanup: 
https://gitlab.com/lilypond/lilypond/-/merge_requests/251



We can take it one bit at a time. In particular, if you can provide a
recipe that would reformat diffs, I can integrate that in my
pre-commit hook, so all new code is compliant.


I don't have that in my toolbox, sorry. The issue is that tools like 
autopep8
and YAPF work on ASTs, so they can't cope with incomplete bits of code. 
I don't
know a safe way to pick just the part of the generated diff that deals 
with what

you change (if you see what I mean).


On Wed, Jul 1, 2020 at 2:03 PM Jean Abou Samra  wrote:
to me it's a no-brainer to introduce automatic formatting. I don't
really care what standard it enforces, as long as it is automated.


I would consider it, but it isn't a no-brainer to me: every automatic 
formatting
tool has its issues, and I prefer formatting my code myself to format 
the way I
think most human-readable.Anyway, let's rather discuss this later when 
we come to

adding a linter to CI.


- Use `string` instead of `str` as an identifier − `str` being a
built-in type
already.

This would be a good rule to follow if the Python team doesn't keep
adding identifiers to the top level scope. For example, in other
projects I had a lot of code become non-compliant overnight when
Python decided that binary formatting of number is worthy of the
toplevel identifier 'bin'.

I agree that Python evolves rapidly, but I think that `str` is common enough
to warrant not using as a variable since you could need it as its actual 
built-in
value in your function. On the other hand, I didn't listen to pylint 
complaining

that we redefined the builtin `credits`.

Cheers,
Jean

Re: Python coding style

[Han-Wen Nienhuys]

On Wed, Jul 1, 2020 at 2:03 PM Jean Abou Samra  wrote:
> 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:

to me it's a no-brainer to introduce automatic formatting. I don't
really care what standard it enforces, as long as it is automated.

> - Use `string` instead of `str` as an identifier − `str` being a
> built-in type
> already.

This would be a good rule to follow if the Python team doesn't keep
adding identifiers to the top level scope. For example, in other
projects I had a lot of code become non-compliant overnight when
Python decided that binary formatting of number is worthy of the
toplevel identifier 'bin'.

> Any thoughts?

We can take it one bit at a time. In particular, if you can provide a
recipe that would reformat diffs, I can integrate that in my
pre-commit hook, so all new code is compliant.

You can also do a global cleanup. We have done this in the past, but
it has the disadvantage that it makes history (eg. git-blame) harder
to read.

-- 
Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen

Re: Python coding style

[Noeck]

For automatic PEP8 formatting, there is

autopep8: https://pypi.org/project/autopep8/ and
yapf: https://pypi.org/project/yapf/

among others.

Cheers,
Joram

Re: Python coding style

[Carl Sorensen]

On Wed, Jul 1, 2020 at 10:55 AM Jean Abou Samra  wrote:

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


pylint shows the problems, but doesn't fix them.  I think that it would be
much better to have an automatic formatter to fix whitespace errors than to
just catch them with pylint.

So I would expect the CI to do something like:

RegularizeWhitespace(python.file)
return(pylint(python.file))

IMO it's a waste of developer brain cells to try to make sure the
whitespace rules for *any* coding standard are followed.  Tools can (and
should) do that.

Expecting developers to make variable names follow standards is fine.  I
have no problem with pylint rejecting a piece of code because we have bad
variable names.  But that's because it's not appropriate (IMO) to have an
automated tool changing variable names.

In short, I think pylint is part of the solution (identifying bad python
code), but is not a complete solution (ensuring whitespace is compliant
with PEP8).

Thanks,

Carl

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

Re: Python coding style

[Carl Sorensen]

Hi Jean,

On Wed, Jul 1, 2020 at 6:03 AM Jean Abou Samra  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.


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


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

Thanks,

Carl

Re: python coding style question

[Jan Nieuwenhuizen]

Op donderdag 01-01-2009 om 09:56 uur [tijdzone +0100], schreef Werner
LEMBERG:

 
 ?  I could imagine that the latter gets executed faster, at least if
 no .pyc files are created.

Faster is not interesting, most of the time.

 Is this just convenience or is this a question of coding style?

It is much more clear to read and thus less error prone.  IMHO, of
course.

Jan.

-- 
Jan Nieuwenhuizen jann...@gnu.org | GNU LilyPond - The music typesetter
http://www.xs4all.nl/~jantien   | http://www.lilypond.org



___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: python coding style question

[Han-Wen Nienhuys]

On Thu, Jan 1, 2009 at 6:56 AM, Werner LEMBERG w...@gnu.org wrote:

 ?  I could imagine that the latter gets executed faster, at least if
 no .pyc files are created.

 Is this just convenience or is this a question of coding style?

The %s style gets hairy when there are lots of substitutions, because
the %s and its argument must be synchronized.

-- 
Han-Wen Nienhuys - han...@xs4all.nl - http://www.xs4all.nl/~hanwen


___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel