parser: Use #-space-% to allow writing PEP8-compliant Python code by neteler · Pull Request #693 · OSGeo/grass-addons

neteler · 2022-01-31T17:26:16Z

Change #% in addons, parser script header definitions to # % because #% is against PEP8 (each line of a block comment starts with a # and a single space).

This PR follows the changes in OSGeo/grass#1287

Change `#%` in parser script header definition to `# %` because `#%` is against PEP8 (each line of a block comment starts with a `#` and a single space). This PR follows the changes in OSGeo/grass#1287

neteler · 2022-01-31T17:28:36Z

Perhaps anything still to change in .github/workflows/?

anikaweinmann

Looks good for me. Maybe you can now remove https://github.com/OSGeo/grass-addons/blob/grass8/.flake8#L5

neteler · 2022-02-01T21:48:41Z

Looks good for me. Maybe you can now remove https://github.com/OSGeo/grass-addons/blob/grass8/.flake8#L5

Good idea. I have removed it in 1ba196c, let's see...

neteler · 2022-02-01T21:54:17Z

Looks good for me. Maybe you can now remove https://github.com/OSGeo/grass-addons/blob/grass8/.flake8#L5

Good idea. I have removed it in 1ba196c, let's see...

Nice attempt but failing: all comments are now rejected.

...
./src/raster/r.rock.stability/r.rock.stability.py:59:1: E265 block comment should start with '# '
################ouput######################
^
./src/raster/r.rock.stability/r.rock.stability.py:229:5: E265 block comment should start with '# '
    ############################SMR_wedge###################
    ^
./src/raster/r.rock.stability/r.rock.stability.py:340:5: E265 block comment should start with '# '
    ############################SSPC#####################
    ^
...

I'll re-instate flake8 E265.

wenzeslaus · 2022-02-02T05:05:57Z

Looks good for me. Maybe you can now remove https://github.com/OSGeo/grass-addons/blob/grass8/.flake8#L5

Good idea. I have removed it in 1ba196c, let's see...

Nice attempt but failing: all comments are now rejected.

I totally agree that this should lead to removal of E265 from the .flake8 file. It's just not the only thing which is needed.

Not all comment are rejected. It's just the comments with the ######## beauty and comments like #bla bla bla, i.e., exactly the stuff E265 aims to prevent and the reason why we want that check enabled. Here are some tips on resolving the E265 issues:

The ######## cases: Either you don't need them (and, e.g., # Output is sufficient) or you need to organizing your code in a better way (e.g., using functions). Rarely, you might want to write something like Markdown or reStructuredText in the comment, but comply with the "start with # " rule.

The #bla bla bla cases: Just put the space there (# bla bla bla).

wenzeslaus

Thank you. Yes, this should happen for the grass8 branch and I see no no suspicious lines using grep -vE "#%|# %|^-$|^\+$|^ " 693.diff. Edit: And there is nothing to see when you do Files changed > settings gear wheel > Hide whitespace > Apply and reload, so that's good.

neteler · 2022-02-02T17:49:58Z

Not all comment are rejected. It's just the comments with the ######## beauty and comments like #bla bla bla, i.e., exactly the stuff E265 aims to prevent and the reason why we want that check enabled.

Ok, I can try to fix the code in order to eventually remove E265.

But at time I don't get flake8 locally running (it complains about old Python style code) which surprises me a bit since we had polished it all... Now, how to run flake8 locally?

Example for the problem I face:

flake8 --config .flake8 --count --statistics --show-source .
multiprocessing.pool.RemoteTraceback: 
"""
Traceback (most recent call last):
  File "/usr/lib/python3.10/site-packages/flake8/checker.py", line 478, in run_ast_checks
    ast = self.processor.build_ast()
  File "/usr/lib/python3.10/site-packages/flake8/processor.py", line 225, in build_ast
    return ast.parse("".join(self.lines))
  File "/usr/lib64/python3.10/ast.py", line 50, in parse
    return compile(source, filename, mode, flags,
  File "<unknown>", line 107
    except Exception, e:
           ^^^^^^^^^^^^
SyntaxError: multiple exception types must be parenthesized
...

I do not plan to fix tons of Python addons to get this PR merged... :)

wenzeslaus · 2022-02-02T18:28:08Z

...I do not plan to fix tons of Python addons to get this PR merged... :)

I don't think you should. Just merge this as is now.

Ok, I can try to fix the code in order to eventually remove E265.

That would be great. Thank you.

But at time I don't get flake8 locally running

Your flake8 command works for me with Flake8 3.8.3 (mccabe: 0.6.1, pycodestyle: 2.6.0, pyflakes: 2.2.0) CPython 3.8.10 on Linux (flake8 --version). So perhaps this happens only with Python 3.10.

...it complains about old Python style code...surprises me a bit since we had polished it all...

We are running Python 3.8 in the CI, so Python 3.10 may behave differently.

The particular error is traceback with a SyntaxError rather than a Flake8 warning.

For me, even flake8 --isolated does not catch some of the issue, but it catches different syntax errors including except Exception, e:. This is the command:

flake8 --isolated --ignore=E,W,F --enable=E999 --count --statistics --show-source

For you, it seems this will fail with traceback instead of reporting all the issues.

In the CI, we don't have an explicit check for Python 3 compatibility other than running Flake8 (which should be enough), but the E999 SyntaxError warning is disabled.

Now, how to run flake8 locally?

I think you are doing it right. Yes, it should report a Flake8 error message, not a traceback, but that might be a bug in Flake8 given that it reports other syntax errors without a traceback. A workaround is to "grep" for it.

Given all this, enabling E999 in the CI (and fixing whatever is needed to make it pass) seems like a good next step.

neteler · 2022-02-02T21:36:40Z

...I do not plan to fix tons of Python addons to get this PR merged... :)

I don't think you should. Just merge this as is now.

Ok, I can try to fix the code in order to eventually remove E265.

That would be great. Thank you.

I tried on a different machine with Python 3.9: no such traceback errors. Seems that Python 3.10 changes things also in the flake8 world....

However, removing E265 leads to 182 errors, i.e.

183 E265 block comment should start with '# '

which should be addressed in a separate PR in my opinion.

wenzeslaus · 2022-02-03T02:13:39Z

...removing E265 leads to 182 errors...which should be addressed in a separate PR in my opinion.

I think there is some misunderstanding. I never suggested to remove E265 from .flake8 in this PR and I already approved this PR. @anikaweinmann did suggest to remove E265, but in a "approving" review. Thus, I interpret @anikaweinmann's suggestion as suggestion only, not a request. I think it was a good suggestion and I agree that E265 should be removed eventually, but clearly it was beyond the scope of this PR.

...I do not plan to fix tons of Python addons to get this PR merged... :)

I don't think you should. Just merge this as is now.

To clarify: I'm all for merging this PR now.

anikaweinmann · 2022-02-03T07:28:44Z

Yes it was only a suggestion and the hope that not to much has to be changed for the remove of E265. Since this is not the case, please feel free to merge and leave it in.

…Geo#693) * parser: Use #-space-% to allow writing PEP8-compliant Python code Change `#%` in parser script header definition to `# %` because `#%` is against PEP8 (each line of a block comment starts with a `#` and a single space). This PR follows the changes in OSGeo/grass#1287

parser: Use #-space-% to allow writing PEP8-compliant Python code

e9f9d36

Change `#%` in parser script header definition to `# %` because `#%` is against PEP8 (each line of a block comment starts with a `#` and a single space). This PR follows the changes in OSGeo/grass#1287

neteler added the Python Related code is in Python label Jan 31, 2022

neteler requested a review from wenzeslaus January 31, 2022 17:26

neteler self-assigned this Jan 31, 2022

anikaweinmann approved these changes Feb 1, 2022

View reviewed changes

.flake8: remove E265, # block comment should start with '# '

1ba196c

restore E265, # block comment should start with '# ' (revert 1ba196c)

5835d68

wenzeslaus approved these changes Feb 2, 2022

View reviewed changes

nilason mentioned this pull request Feb 2, 2022

r.forcircular: Evaluation of circular bioconomy level of forest ecosystems #694

Merged

neteler merged commit f17c792 into OSGeo:grass8 Feb 3, 2022

neteler deleted the parser_pep8_comment_space branch February 3, 2022 08:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

parser: Use #-space-% to allow writing PEP8-compliant Python code#693

parser: Use #-space-% to allow writing PEP8-compliant Python code#693
neteler merged 3 commits intoOSGeo:grass8from
neteler:parser_pep8_comment_space

neteler commented Jan 31, 2022

Uh oh!

neteler commented Jan 31, 2022

Uh oh!

anikaweinmann left a comment

Uh oh!

neteler commented Feb 1, 2022

Uh oh!

neteler commented Feb 1, 2022

Uh oh!

wenzeslaus commented Feb 2, 2022

Uh oh!

wenzeslaus left a comment •

edited

Loading

Uh oh!

neteler commented Feb 2, 2022

Uh oh!

wenzeslaus commented Feb 2, 2022

Uh oh!

neteler commented Feb 2, 2022

Uh oh!

wenzeslaus commented Feb 3, 2022

Uh oh!

anikaweinmann commented Feb 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

Conversation

neteler commented Jan 31, 2022

Uh oh!

neteler commented Jan 31, 2022

Uh oh!

anikaweinmann left a comment

Choose a reason for hiding this comment

Uh oh!

neteler commented Feb 1, 2022

Uh oh!

neteler commented Feb 1, 2022

Uh oh!

wenzeslaus commented Feb 2, 2022

Uh oh!

wenzeslaus left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neteler commented Feb 2, 2022

Uh oh!

wenzeslaus commented Feb 2, 2022

Uh oh!

neteler commented Feb 2, 2022

Uh oh!

wenzeslaus commented Feb 3, 2022

Uh oh!

anikaweinmann commented Feb 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wenzeslaus left a comment •

edited

Loading