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

fix code style issues in easybuild.tools + add flake8 linting test #3282

Merged
merged 6 commits into from
May 19, 2020

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Apr 17, 2020

First step towards getting flake8 pass for the whole repo to inspect what autopep8 can do.

Command run: autopep8 -r -i --max-line-length 120 easybuild/tools and autopep8 -a -r -i --max-line-length 120 easybuild/tools

Note: Don't use aggressive level 2 (-aa) as this will change if x == True to if x which is a semantic change: hhatto/autopep8#539

@boegel
Copy link
Member

boegel commented Apr 17, 2020

@Flamefire This should be accompanied by running flake8 on the list of files that should now fully comply with PEP8 in the tests, to ensure future PRs don't introduce regressions that diverge from PEP8 again?

@boegel boegel added this to the next release (4.2.1?) milestone Apr 17, 2020
@Flamefire Flamefire force-pushed the autopep8-first branch 2 times, most recently from d0e0e31 to 26bad44 Compare April 17, 2020 10:25
@Flamefire
Copy link
Contributor Author

@boegel Done. Again I'm using a separate job (even a separate yaml file) to make it cleaner and run only once. For flake8 it doesn't make sense to run it with multiple pythons. latest is enough.

Also see the name. IMO "Static Analysis / python-linting" is way more concise then the rather verbose "EasyBuild framework unit tests / build" which hides the more important parts. I'd suggest renaming those to framework / unit tests

Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

PRs like this should definitely not get bigger than this one is...

Although the changes were not done manually, they should still be carefully reviewed, and surprises may pop up...

easybuild/tools/asyncprocess.py Show resolved Hide resolved
.github/workflows/linting.yml Show resolved Hide resolved
easybuild/tools/configobj.py Show resolved Hide resolved
easybuild/tools/filetools.py Outdated Show resolved Hide resolved
easybuild/tools/job/gc3pie.py Show resolved Hide resolved
easybuild/tools/job/pbs_python.py Outdated Show resolved Hide resolved
easybuild/tools/job/pbs_python.py Show resolved Hide resolved
easybuild/tools/run.py Show resolved Hide resolved
easybuild/tools/toolchain/toolchain.py Show resolved Hide resolved
easybuild/tools/variables.py Show resolved Hide resolved
@Flamefire
Copy link
Contributor Author

@boegel Added a couple of comments. For the remaining ones: Can we agree to keep this series of PRs focused on PEP8 compliance so flake8 can be enabled? If one is supposed to improve/fix/change every code piece that is neighbouring to code touched by autopep8 we won't finish anytime soon.

So I would really prefer to just fix the PEP8 failures and only change where it is useful for pep8 or an introduced regression.

Examples:

  • Ordering of imports: OK to redo as introduced here
  • is_string instead of isinstance(regExp, str):: actual change in semantic, so not OK as we would have to check if it is really ok to include python 2 strings
  • variable renaming: definitely out of scope of this PR

I'm a fan of the boy scout rule but not for a (semi-)automatic style fix which is big enough already.

Final note on e.g. variable renaming: Linters can be used to detect single-letter variables too. This could be done after flake8 passes :)

@boegel
Copy link
Member

boegel commented Apr 20, 2020

@Flamefire I understand that you want to avoid that the scope of this PR keeps expanding, so feel free to disagree with some suggested changes (and maybe consider opening a separate PR (or issue) for them).

Agreed on is_string vs isinstance(xxx, str), it's probably fine to leave it as is in hindsight.

W.r.t. single-letter variables outside of list comprehensions: OK to consider outside the scope of this PR. Do you know of a linter that has a check for this specifically?

@Flamefire
Copy link
Contributor Author

Ok then I'll define the scope as to not introduce new issues. This should be easily checkable.

There is a flake8 plugin which checks general naming: https://pypi.org/project/pep8-naming/

And more generically (superset of that plugin): pylint. http://pylint.pycqa.org/en/latest/user_guide/options.html See e.g. https://stackoverflow.com/questions/21833872/why-does-pylint-object-to-single-character-variable-names

@Flamefire
Copy link
Contributor Author

Closes all open discussions but 1 either by fixing or mentioning cause for not fixing according to above. Need your call on how to handle #3282 (comment)

@boegel boegel changed the title Run autopep8 over tools folder fix code style issues in easybuild.tools + add flake8 linting test May 19, 2020
@boegel boegel merged commit eb66a53 into easybuilders:develop May 19, 2020
@Flamefire Flamefire deleted the autopep8-first branch May 19, 2020 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants