Re: black: Python code formatter (eg PEP8)

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

Re: black: Python code formatter (eg PEP8)

Panagiotis Mavrogiorgos
Hello all,
 
The main problem with adopting style checkers so late in a project's life is that they usually introduce really vast changes. You practically end up with a huge commit that touches each and every python file. Needless to say this makes using e.g. git blame much much harder. 

If you want to test it out, please try the following from a clean state:

cd lib/python 
git status     # make sure that no files have any changes in lib/python
black --exclude OBJ ./
git diff --patience --minimal ./ | wc -l

That's a 75k+ line diff and that's only the "grass" library. The gui should give something similar.

To restore the repo, just run:

git checkout ./

That being said, I do use black in practically all my new projects, usually applying it via pre-commit, and it works great, but I am not sure if it is truly an option for GRASS.

all the best,
P.

_______________________________________________
grass-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/grass-dev
Reply | Threaded
Open this post in threaded view
|

Re: black: Python code formatter (eg PEP8)

wenzeslaus


On Wed, May 29, 2019 at 4:07 AM Panagiotis Mavrogiorgos <[hidden email]> wrote:
 
The main problem with adopting style checkers so late in a project's life is that they usually introduce really vast changes. You practically end up with a huge commit that touches each and every python file. Needless to say this makes using e.g. git blame much much harder.

This already happened in the past:

C
286,279 additions and 272,386 deletions

Python
47,846 additions and 34,489 deletions



_______________________________________________
grass-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/grass-dev
Reply | Threaded
Open this post in threaded view
|

Re: black: Python code formatter (eg PEP8)

Markus Neteler
On Thu, May 30, 2019 at 1:14 AM Vaclav Petras <[hidden email]> wrote:

> On Wed, May 29, 2019 at 4:07 AM Panagiotis Mavrogiorgos <[hidden email]> wrote:
>>
>> The main problem with adopting style checkers so late in a project's life is that they usually introduce really vast changes. You practically end up with a huge commit that touches each and every python file. Needless to say this makes using e.g. git blame much much harder.
>
>
> This already happened in the past:
>
> C
> https://trac.osgeo.org/grass/changeset/32526
> 286,279 additions and 272,386 deletions

In 2006, there was another major change in the C code (with months of
preparation):

ANSIfication of GRASS C functions (automated reformatting from K & R C
to ANSI C):
https://lists.osgeo.org/pipermail/grass-dev/2006-January/020767.html
- https://trac.osgeo.org/grass/changeset/18618,
https://trac.osgeo.org/grass/changeset/18619
- https://trac.osgeo.org/grass/changeset/18623,
https://trac.osgeo.org/grass/changeset/18625
- https://trac.osgeo.org/grass/changeset/18626,
https://trac.osgeo.org/grass/changeset/18627
- https://trac.osgeo.org/grass/changeset/18628,
https://trac.osgeo.org/grass/changeset/18632
- https://trac.osgeo.org/grass/changeset/18633

Related scientific publication:
A Feedback Based Quality Assessment to Support Open Source Software
Evolution: the GRASS Case Study
https://www.researchgate.net/publication/224674480_A_Feedback_Based_Quality_Assessment_to_Support_Open_Source_Software_Evolution_the_GRASS_Case_Study

(btw: I didn't find these changesets in the git commit list)

> Python
> https://trac.osgeo.org/grass/changeset/68374
> 47,846 additions and 34,489 deletions


best,
Markus


--
Markus Neteler, PhD
https://www.mundialis.de - free data with free software
https://grass.osgeo.org
https://courses.neteler.org/blog
_______________________________________________
grass-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/grass-dev
Reply | Threaded
Open this post in threaded view
|

Re: black: Python code formatter (eg PEP8)

Panagiotis Mavrogiorgos
In reply to this post by Panagiotis Mavrogiorgos
Just few more points (not in any particular order):

- If black is used, all open pull requests, branches etc will need to be rebased. My guess is that it will often be easier to just re-implement instead of solving the conflicts.
- Unless black is used on releasebranches too, backporting will not be trivial and/or automated.
- black uses 88 chars as the default line length. For legacy python this is usually enough, but for newer code using type annotations you often want to bump that up to 100 or even 120 chars
- black formats functions/methods with a lot of arguments in a specific way that may not be to everyone's liking. E.g.

def foo(aaaaaaaaaaaa, bbbbbbbbbbbb, cccccccccccccccc, ddddddddddddddddddd):
    pass

becomes:

def foo(
    aaaaaaaaaaaa,
    bbbbbbbbbbbb,
    cccccccccccccccc,
    ddddddddddddddddddd,
):
    pass
 
bumping up the line length, somewhat's mitigates this.

- black is an automated tool. Optimal results are usually achieved by manually adjusting the code and/or writing the code in a way that is compatible with black (i.e. black will not change it). a typical example - and there are more - are double list comprehensions (which IMHV should usually be avoided but that's not really relevant here). E.g:

words = [word.upper() for sentence in paragraph for word in sentence]

becomes:

words = [
    word.upper()
    for sentence in paragraph
    for word in sentence
]

Re-rewriting this as a nested loop is one less line of code, is more readable because it doesn't change the flow of execution, is more expendable and is obviously more familiar to those who don't know python that well:

words = []
for sentence in paragraph:
    for word in sentence:
        words.append(word.upper()) 
 
For the record, this particular case could be re-written using itertools.chain

from itertools import chain 

words = [word.upper() for word in chain(*paragraph)]

- black doesn't play too well with inline comments. E.g. 

a = [
    "a",
    "b",  # noqa
    "c", 
]

will be transformed to:

a = ["a", "b", "c"]  # noqa

This means that the noqa comment will be applied to all the arguments instead of "b" only. 

- Probably not a huge issue for GRASS, but black does not preserve indentation of comments. So E.g this will become:

# class A(object):
    # def __init__(self):
        # self.a = 1
        # self.b = 2 

 will become

# class A(object):
# def __init__(
self):
# self.a = 1
# self.b = 2 

If you want to preserve the indentation you would need to comment out using this:

# class A(object):
#    def __init__(self):
#        self.a = 1
#        self.b = 2 
 
all the best,
P.

_______________________________________________
grass-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/grass-dev