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

r.topidx: Rewrite Perl scripts in Python 3 #766

Merged
merged 7 commits into from
Feb 18, 2021

Conversation

HuidaeCho
Copy link
Member

Address #762.

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

Thanks. That's a (very!) good solution of #762. I think I lack context to fully review this. (Documentation, tests, ... I'm not saying you need to add them to get this merged since the goal is just to get #762 resolved and for that the current extent of the PR is more than enough.)

As for general Python, just apply Black and then use Flake8 with default settings. From other things, I see only f-strings, but I think we can already allow them in "secondary" code like this (so just leave those there).

@wenzeslaus
Copy link
Member

On a second thought, f-strings are probably okay for these scripts, just applying Black and checking the code with Flake8 should be enough.

@neteler
Copy link
Member

neteler commented Nov 17, 2020

@HuidaeCho do you plan to make further modifications?

@HuidaeCho
Copy link
Member Author

@HuidaeCho do you plan to make further modifications?

@neteler No, I think this is it. I'm not familiar with Black and Flake8.

@neteler
Copy link
Member

neteler commented Nov 17, 2020

flake8 reports this:

./arc.to.gridatb.py:16:60: E703 statement ends with a semicolon
    print('Usage: arc.to.gridatb.py arc_file gridatb_file');
                                                           ^

I'll fix it here.

@neteler
Copy link
Member

neteler commented Nov 17, 2020

Do you want any other changes @wenzeslaus (since the PR is still in "changes requested" mode)?

@wenzeslaus
Copy link
Member

Do you want any other changes @wenzeslaus (since the PR is still in "changes requested" mode)?

I was suggesting only to use Flake8 and Black which seems you did to at least some extent. Fully applying Black would be good.

@neteler
Copy link
Member

neteler commented Nov 17, 2020

(Please (someone) add the magic black cmd line to

https://trac.osgeo.org/grass/wiki/Submitting/Python )

@wenzeslaus
Copy link
Member

Please (someone) add the magic black cmd line to

See Style > Automation at https://trac.osgeo.org/grass/wiki/Submitting/Python#Automation

Co-authored-by: Markus Neteler <neteler@gmail.com>
@neteler
Copy link
Member

neteler commented Nov 20, 2020

I suggest to merge. The black formatting result looks strange to me (tried locally).

@HuidaeCho
Copy link
Member Author

I suggest to merge. The black formatting result looks strange to me (tried locally).

@neteler Agreed with you. The black result looks unnatural. I just made small changes to make flake8 happy though.

@wenzeslaus
Copy link
Member

I suggest to merge. The black formatting result looks strange to me (tried locally).

@neteler Agreed with you. The black result looks unnatural. I just made small changes to make flake8 happy though.

The place to have this discussion is #543. Speak now or forever hold your peace.

@wenzeslaus
Copy link
Member

Since I'm introducing Black elsewhere (#1347) and we have already used some Black here and there, I have applied Black and renamed files to use underscores instead of dots to separate words. Example of what I consider an improvement:

- if len(sys.argv) == 1 or len(sys.argv) == 4 or len(sys.argv) > 5 or \
-    re.match('^-*help', sys.argv[1]):
-     print('Usage: gridatb.to.arc.py gridatb_file arc_file'
-           ' [xllcorner yllcorner]')
+ if (
+     len(sys.argv) == 1
+     or len(sys.argv) == 4
+     or len(sys.argv) > 5
+     or re.match("^-*help", sys.argv[1])
+ ):
+     print("Usage: gridatb.to.arc.py gridatb_file arc_file [xllcorner yllcorner]")

The (...): piece seems strange at the beginning, but at the end easier to read than that one space difference in indentation and backslash in the previous code because number of rules applied is smaller. It is worth mentioning that the Black formatting is also meant for version control (subsequent changes to the code should generate small diffs in the sense that changing one or ... touches only one line and no other part of the condition).

@wenzeslaus wenzeslaus added this to In progress in Joint OGC OSGeo ASF Code Sprint 2021 via automation Feb 17, 2021
@HuidaeCho
Copy link
Member Author

+1 for merging it whenever it's ready. I don't worry too much about how the code looks.

@wenzeslaus wenzeslaus merged commit c402ca5 into OSGeo:master Feb 18, 2021
Joint OGC OSGeo ASF Code Sprint 2021 automation moved this from In progress to Done Feb 18, 2021
@HuidaeCho HuidaeCho deleted the r_topidx_scripts branch March 9, 2021 23:16
marisn pushed a commit to marisn/grass that referenced this pull request Mar 22, 2021
wenzeslaus added a commit to wenzeslaus/grass that referenced this pull request Apr 1, 2021
This enables Perl in Super-Linter check since local perlcritic does not give any errors after OSGeo#766 and OSGeo#1431.
wenzeslaus added a commit that referenced this pull request Apr 1, 2021
This enables Perl in Super-Linter check (uses plain perl) since local perlcritic does not give any errors after #766 and #1431.
@neteler neteler added this to the 8.0.0 milestone Dec 9, 2021
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022
This enables Perl in Super-Linter check (uses plain perl) since local perlcritic does not give any errors after OSGeo#766 and OSGeo#1431.
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
This enables Perl in Super-Linter check (uses plain perl) since local perlcritic does not give any errors after OSGeo#766 and OSGeo#1431.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants