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

Code quality fixes for grass package (lib/python) #576

Merged

Conversation

wenzeslaus
Copy link
Member

@wenzeslaus wenzeslaus commented May 2, 2020

This PR fixes Flake8 errors/warnings:

  • Do not assign a lambda expression, use a def (E731)
  • 'raise NotImplemented' should be 'raise NotImplementedError' (F901)
  • Redefinition of unused...from line... (F811)
  • Do not compare types, use 'isinstance()' (E721)
  • Module level import not at top of file (E402)

And additionally it unifies checks for Python 3 using sys.version_info.major.

I also moved serious errors to be resolved up in the Flake8 configuration file and whitespace errors down.

For review, you can check the changes in individual commits one by one.

Fixing by actually moving imports (hashlib), moving the non-importing code down
after the imports (matplotlib.use and Python 2/3 variables).

Several files ignored using the per-file-ignores. These may or may not be substantiated as exceptions,
so keeping them in the config file rather than in the file itself to have here a clear warning
that the check was disabled.

In general E402 should be fixable by moving code around and couple inline ignores like in the case
of matplotlib.use in gunittest/multireport.
Now all checks for Python 3 which are using sys.version_info.major are using >= operator
because we are actually interested in 'not Python 2' and we assume Python 4 will behave more like
Python 3 and not like Python 2.
@wenzeslaus wenzeslaus marked this pull request as draft May 2, 2020 03:42
This seems to mostly report things imported twice.
NotImplemented is a constant for implementing binary special methods.
NotImplementedError is an exception indicating real implementation still needs to be added.
Lambdas are for embedding into larger expressions. When assigned,
it is just a strange syntax for a function definition
because def can appear anywhere in the code already.
@wenzeslaus wenzeslaus requested a review from ninsbl May 2, 2020 19:45
@wenzeslaus wenzeslaus marked this pull request as ready for review May 2, 2020 19:46
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.

I have not applied and tested, but the changes look good to me.
I appreciate the work on code standards. Being no programer by education I feel tools like black or flake8 can be quite helpful.

@wenzeslaus
Copy link
Member Author

Thank you. Reading is all I expected. I think it is sufficient and here possibly more reliable than testing it (what to test would be quite tricky to determine in some cases).

@wenzeslaus wenzeslaus merged commit ad07993 into OSGeo:master May 3, 2020
@wenzeslaus wenzeslaus deleted the code-quality-fixes-for-lib-python branch May 3, 2020 22:54
@neteler neteler added this to the 8.0.0 milestone Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants