Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Fix formatting, use Black #543

Closed
wenzeslaus opened this issue Apr 23, 2020 · 8 comments
Closed

[Bug] Fix formatting, use Black #543

wenzeslaus opened this issue Apr 23, 2020 · 8 comments
Labels
bug Something isn't working

Comments

@wenzeslaus
Copy link
Member

Describe the bug

There is over 5000 whitespace issues in Python source code in GRASS GIS. Unlike other languages, Python has good standards for whitespace (even the one which does not matter syntactically) such as PEP8. Let's use them.

Black (https://github.com/psf/black) is a Python code formatter which cannot be configured, so even the parts which are open to interpretation in PEP8 are fixed in Black. So, there is no need to configure it (except line length, string handling, and Python version specific code handling - compare e.g. to ClangFormat or Pylint). As one testimonial says: Black is opinionated so you don't have to be.

Depending on the parameters, the command line tool black reformats code automatically, checks if it is correctly formatted, or gives diff saying how code should be changed, so we can use it reformat code once and then have it in CI to check the formatting and suggest the fix.

Some choices made in Black are different from what we are using currently, but I think going with the standard is just worth it and also easy enough since the tool will just take care of that (it can run from command line, your editor may have an integration already, or you can set it up as a pre-commit hook.

Black is still in beta, so some changes in formatting may occur, but my guess is that it will be much smaller than the current changes needed or even changes done in past.

Also this will introduce many changes in the source code, so e.g. git blame will require more work to get to the actual author. However, we need to make a lot of changes anyway (see e.g. #538. #539, #540) and we have several of these things in the souce code already (given the history of the project).

To Reproduce

  1. Go to a desired directory.
  2. Delete all whitespace-related errors to ignore from .flake8 config file (that's the second part of the list in each file).
  3. Run flake8 --statistics --count ..
  4. See the many errors.

Expected behavior

Contributors can use Python editors with default settings and don't need to manually get the formatting right when there is a tool for it.

Current error reports by Flake8

lib/python

9     E117 over-indented
3     E121 continuation line under-indented for hanging indent
42    E122 continuation line missing indentation or outdented
14    E123 closing bracket does not match indentation of opening bracket's line
2     E124 closing bracket does not match visual indentation
2     E125 continuation line with same indent as next logical line
11    E126 continuation line over-indented for hanging indent
42    E127 continuation line over-indented for visual indent
89    E128 continuation line under-indented for visual indent
26    E129 visually indented line with same indent as next logical line
6     E201 whitespace after '{'
7     E202 whitespace before ')'
156   E203 whitespace before ':'
4     E211 whitespace before '('
183   E221 multiple spaces before operator
10    E222 multiple spaces after operator
55    E225 missing whitespace around operator
124   E226 missing whitespace around arithmetic operator
51    E228 missing whitespace around modulo operator
82    E231 missing whitespace after ':'
180   E241 multiple spaces after ','
185   E251 unexpected spaces around keyword / parameter equals
22    E261 at least two spaces before inline comment
3     E271 multiple spaces after keyword
3     E272 multiple spaces before keyword
3     E301 expected 1 blank line, found 0
77    E302 expected 2 blank lines, found 1
17    E303 too many blank lines (3)
57    E305 expected 2 blank lines after class or function definition, found 1
4     E401 multiple imports on one line
722   E501 line too long (90 > 88 characters)
12    W291 trailing whitespace
2     W292 no newline at end of file
20    W293 blank line contains whitespace
9     W391 blank line at end of file
14    W503 line break before binary operator
55    W504 line break after binary operator
2303

scripts

1     E121 continuation line under-indented for hanging indent
1     E125 continuation line with same indent as next logical line
14    E127 continuation line over-indented for visual indent
14    E128 continuation line under-indented for visual indent
1     E202 whitespace before ')'
2     E211 whitespace before '['
10    E221 multiple spaces before operatorpmav99
5     E225 missing whitespace around operator
1     E226 missing whitespace around arithmetic operator
35    E231 missing whitespace after ','
28    E251 unexpected spaces around keyword / parameter equals
7     E261 at least two spaces before inline comment
3     E271 multiple spaces after keyword
3     E272 multiple spaces before keyword
9     E302 expected 2 blank lines, found 1
5     E303 too many blank lines (2)
60    E305 expected 2 blank lines after class or function definition, found 1
340   E501 line too long (109 > 88 characters)
5     W293 blank line contains whitespace
2     W503 line break before binary operator
19    W504 line break after binary operator
565

temporal

4     E117 over-indented
3     E125 continuation line with same indent as next logical line
2     E126 continuation line over-indented for hanging indent
38    E127 continuation line over-indented for visual indent
23    E128 continuation line under-indented for visual indent
1     E129 visually indented line with same indent as next logical line
2     E222 multiple spaces after operator
24    E225 missing whitespace around operator
2     E226 missing whitespace around arithmetic operator
48    E228 missing whitespace around modulo operator
37    E231 missing whitespace after ','
39    E241 multiple spaces after ','
26    E251 unexpected spaces around keyword / parameter equals
2     E271 multiple spaces after keyword
14    E302 expected 2 blank lines, found 1
3     E303 too many blank lines (2)
45    E305 expected 2 blank lines after class or function definition, found 1
251   E501 line too long (95 > 88 characters)
12    W291 trailing whitespace
22    W293 blank line contains whitespace
4     W391 blank line at end of file
1     W504 line break after binary operator
603

gui/wxpython

5     E117 over-indented
22    E122 continuation line missing indentation or outdented
2     E123 closing bracket does not match indentation of opening bracket's line
4     E124 closing bracket does not match visual indentation
1     E125 continuation line with same indent as next logical line
12    E126 continuation line over-indented for hanging indent
83    E127 continuation line over-indented for visual indent
56    E128 continuation line under-indented for visual indent
3     E131 continuation line unaligned for hanging indent
3     E202 whitespace before '}'
3     E203 whitespace before ':'
1     E211 whitespace before '('
7     E222 multiple spaces after operator
11    E225 missing whitespace around operator
4     E226 missing whitespace around arithmetic operator
4     E231 missing whitespace after ','
2     E241 multiple spaces after ','
13    E261 at least two spaces before inline comment
7     E271 multiple spaces after keyword
6     E272 multiple spaces before keyword
1     E301 expected 1 blank line, found 0
15    E302 expected 2 blank lines, found 1
14    E303 too many blank lines (3)
41    E305 expected 2 blank lines after class or function definition, found 1
2     E306 expected 1 blank line before a nested definition, found 0
1108  E501 line too long (96 > 88 characters)
22    W291 trailing whitespace
23    W293 blank line contains whitespace
15    W503 line break before binary operator
537   W504 line break after binary operator
2027

Additional context

Black was already suggested by @neteler on grass-dev with some discussion by @pmav99 and myself.

CI (GitHub Actions) now has Flake8 checks with many issues now being fixes, so producing high-quality code should become easier for contributors also in the other areas than consistent formatting as the existing code will obey the rules and best practices.

@wenzeslaus wenzeslaus changed the title [Bug] [Bug] Fix formatting, use Black Apr 23, 2020
@wenzeslaus
Copy link
Member Author

wenzeslaus commented Apr 23, 2020

Things which would be good to do before applying Black to the code base:

  • Fix Flake8 errors related to inconsistent indentation and tabs
    • This was an issue with applying autopep8 to wxGUI code base as autopep8 got confused by mixed tabs and spaces for indentation which resulted in broken code.
    • This is almost done, temporal directory needs to be fixed and also testsuite directories.
  • Minimize non-whitespace Flake8 errors
    • Let's not risk that some obsolete Python 2 syntax confuses Black.
    • A big chunk of this is done too. Perhaps we can consider it done even now. See the remaining errors in .flake8 files (find . -name .flake8) or get stats by removing the errors from configuration and running flake8 --statistics ..
  • Merge, close, or (likely just) minimize number of PRs touching Python code to avoid need for rebasing/merging the changes.

What code should be Black applied to? The above stats are just about the "primary" code such as modules in scripts directory, but Black (and subsequent checks) can be applied to helper code as well, namely tests, tools, manual building Python scripts.

@landam landam added the bug Something isn't working label Apr 24, 2020
@neteler
Copy link
Member

neteler commented Apr 24, 2020

Thanks for your hard work on getting GH Actions to work and on the code style formatting issues.

While I am in favour of significantly improving the Python code formatting, a related question: do we backport those changes? If not, backporting will soon be hard or even impossible. Or, do we fork 7.10 from master in some near future?

@neteler
Copy link
Member

neteler commented Apr 26, 2020

Another question: will/can we have this in https://github.com/OSGeo/grass-addons/ as well?

@wenzeslaus
Copy link
Member Author

...a related question: do we backport those changes? If not, backporting will soon be hard or even impossible.

Given our current plans, if we do it before branching out to do the 8.0.0 release, we are good.

@wenzeslaus
Copy link
Member Author

Will/can we have this in https://github.com/OSGeo/grass-addons/ as well?

I suggest to ask for full back Black and maximum Flake8 compliance for all new addons.

Additionally, we could apply Black before/when reorganizing addons for v8.

@wenzeslaus
Copy link
Member Author

Black applied to the script directory in #1347.

@wenzeslaus
Copy link
Member Author

Black applied to the lib/init/grass.py file in #1359.

@wenzeslaus
Copy link
Member Author

Black 20.8b1 with target versions py36, py37, py38 and line length 88 applied in:
#1527 #1513 #1388 #1387 #1386 #1382 #1366 #1365 #1359 #1347

Configuration is in the pyproject.toml file in the project's root directory. Checked using GitHub Actions (workflow file black.yml).

Bugs in Black 20.8b1 prevent these three files from being formatted correctly (to be resolved later with some subsequent release of Black together with other updates if needed):

python/grass/imaging/images2gif.py
python/grass/pygrass/raster/category.py
gui/wxpython/lmgr/layertree.py

Black is not applied to addons yet (except for some files).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants