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

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

Merged
merged 4 commits into from
Feb 9, 2021

Conversation

wenzeslaus
Copy link
Member

Accept '# %' in parser script header definition because '#%' is against PEP8
(each line of a block comment starts with a # and a single space).

'#%' is still supported and documented as backwards compatibility.

All documentation and generated code uses the new '# %'.

@wenzeslaus
Copy link
Member Author

Review by commits. Most relevant is 9cfc7c0.

@wenzeslaus
Copy link
Member Author

Notably, the first commit in the series works as is without the other three. The first three are reported as failed only because Travis is set to end if there is a new commit on the branch.

Copy link
Member

@ninsbl ninsbl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sound like a reasonable change to me. Good to able to activate all (or at least more) flake8 checks!

Given the number of files changed files, I did not go through all of them, but I could not find any issues in the visible I looked at (pluss some additional where i loaded the diff) and tests are passing. Neither did I find any cases where # % has been in the python code base before, so the change should be safe.
In the light of the extent of changes, maybe pop a note to grass-dev list and point to this PR? If no one objects within two days you merge?

@wenzeslaus wenzeslaus added the enhancement New feature or request label Feb 5, 2021
Accept '# %' in parser script header definition because '#%' is against PEP8
(each line of a block comment starts with a # and a single space).

'#%' is still supported and documented as backwards compatibility.

All documentation and generated code uses the new '# %'.
@wenzeslaus wenzeslaus merged commit 6352241 into OSGeo:master Feb 9, 2021
@wenzeslaus wenzeslaus deleted the parser-pep8-comment branch February 9, 2021 04:13
@neteler
Copy link
Member

neteler commented Mar 8, 2021

I'd like to backport this great PR, any objections? Needed in terms of compatibility to write "modern-style" addons...

neteler added a commit to actinia-org/ace that referenced this pull request Mar 8, 2021
@wenzeslaus
Copy link
Member Author

I guess one could consider being forced to break basic PEP8 rules a bug. Since both core and addon modules work, I think the change matured enough. However, backporting just the g.parser part (general/g.parser/main.c) seems to me better than changing all 150+ files in a patch release. I would prepare a PR for that. What do you think?

@neteler
Copy link
Member

neteler commented Mar 10, 2021

Yes, the backport of just the g.parser part would be nice.

wenzeslaus added a commit to wenzeslaus/grass that referenced this pull request Mar 11, 2021
Accept '# %' in parser script header definition because '#%' is against PEP8
(each line of a block comment starts with a # and a single space).
Only minimal changes to achive this support are included.

'#%' is still supported fully supported and remains in documentation.
The full switch to '# %' is left for v8.

Partial backport of 6352241 (OSGeo#1287).
@wenzeslaus
Copy link
Member Author

the backport of just the g.parser part would be nice.

Done in #1446. Please, let me know there if that's what you had in mind.

@neteler
Copy link
Member

neteler commented Mar 15, 2021

the backport of just the g.parser part would be nice.

Done in #1446. Please, let me know there if that's what you had in mind.

Yes, exactly that - thanks.

wenzeslaus added a commit that referenced this pull request Mar 16, 2021
…1446)

Accept '# %' in parser script header definition because '#%' is against PEP8
(each line of a block comment starts with a # and a single space).
Only minimal changes to achieve this support are included.

'#%' is still supported fully supported and remains in documentation.
The full switch to '# %' is left for v8.

Partial backport of 6352241 (#1287).
marisn pushed a commit to marisn/grass that referenced this pull request Mar 22, 2021
…Geo#1287)

Accept '# %' in parser script header definition because '#%' is against PEP8
(each line of a block comment starts with a # and a single space).

'#%' is still supported and documented as backwards compatibility.

All documentation and generated code uses the new '# %'.

Changes '#%' to '# %' for modules in scripts, gui, and temporal.
@HuidaeCho
Copy link
Member

HuidaeCho commented Jun 12, 2021

@wenzeslaus I'm writing a new module and it's honestly pain to have to type all these extra spaces when defining options and flags because I need to type #-space-%-space-key:-space-name a lot. I tried black, but it didn't add spaces for me. Am I missing any black configs? For now, I use vim mapping. https://unix.stackexchange.com/a/343043, but this is not ideal because any hash will add a space even within code (e.g., strings).

@wenzeslaus
Copy link
Member Author

@wenzeslaus I'm writing a new module and it's honestly pain to have to type all these extra spaces when defining options and flags because I need to type #-space-%-space-key:-space-name a lot.

Let's have this discussion on grass-dev. Perhaps you can continue this thread:

[GRASS-dev] Python and script header definitions for modules
https://lists.osgeo.org/pipermail/grass-dev/2021-April/095098.html

I tried black, but it didn't add spaces for me. Am I missing any black configs?

I think it leaves comments which look too complicated, i.e., could mean be else than a plain sentence, which is correct, just not desired here. (Well, that's at least my guess just based on experience.)

For now, I use vim mapping.

I suggest vi, that's one less character to type. ;-) An old joke I guess...

@HuidaeCho
Copy link
Member

I suggest vi, that's one less character to type. ;-)

Technical correctness? In the end, we're forced to type one more space, so I didn't mind the extra "m", ;-)

@neteler neteler added this to the 7.8.6 milestone Dec 9, 2021
@neteler neteler added the Python Related code is in Python label Dec 9, 2021
neteler added a commit to OSGeo/grass-addons that referenced this pull request Feb 3, 2022
* 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
IvanMarchesini pushed a commit to IvanMarchesini/grass-addons that referenced this pull request Feb 24, 2022
…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
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022
…Geo#1287)

Accept '# %' in parser script header definition because '#%' is against PEP8
(each line of a block comment starts with a # and a single space).

'#%' is still supported and documented as backwards compatibility.

All documentation and generated code uses the new '# %'.

Changes '#%' to '# %' for modules in scripts, gui, and temporal.
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
…Geo#1287)

Accept '# %' in parser script header definition because '#%' is against PEP8
(each line of a block comment starts with a # and a single space).

'#%' is still supported and documented as backwards compatibility.

All documentation and generated code uses the new '# %'.

Changes '#%' to '# %' for modules in scripts, gui, and temporal.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants