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

Use pycodestyle in CI and fix style issues in EasyConfigs #21405

Closed
wants to merge 4 commits into from

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Sep 13, 2024

framework uses pycodestyle if available and pep8 only as a fallback and later likely not at all (pep8 was renamed to pycodestyle and the existing package is no longer updated)
So we need to check for either both or (more future proof) only for pycodestyle.

Install it in CI to actually test it and fix the found issues to make the test pass again.

Requires:

framework uses pycodestyle if available and pep8 only as a fallback and
later likely not at all (pep8 was renamed to pycodestyle and the
existing package is no longer updated)
So we need to check for either both or (more future proof) only for
pycodestyle.
pep8 has bugs which makes it miss many issues like long-lines in docstrings.
This might make the tests fail if both packages are available as
framework prefers the newer pycodestyle.
These make the test fail, so fix them.
@boegel
Copy link
Member

boegel commented Sep 25, 2024

@Flamefire Seems like this needs a bit more love?

You seem to be touching easyconfigs that will no longer be included with EasyBuild v5.0.0, no point in putting effort in those imho...

@Flamefire
Copy link
Contributor Author

@Flamefire Seems like this needs a bit more love?

You seem to be touching easyconfigs that will no longer be included with EasyBuild v5.0.0, no point in putting effort in those imho...

Oh I thought it was only the ambiguous variable name issue.

The problem with not fixing this is that we run the style check over all ECs and if we don't fix the style even in the old one the CI will continue to fail.

Best we can do is ignore or fix the ambiguous variable names and merge the rest even though CI is failing with checksum issues etc. Otherwise we'd need to fix the other issues too which I think won't be to hard but boring ;-)

@boegel
Copy link
Member

boegel commented Sep 30, 2024

Doing this cleanup in develop branch is way too painful, so let's only do it for the 5.0.x branch (which will very soon become develop), see #21502.

It's really only relevant for the 5.0.x branch too, since easybuilders/easybuild-framework#4634 was changed to target 5.0.x

@Flamefire
Copy link
Contributor Author

Ok I see. However I did like the line breaks here more than the ones in #21502 resulting from the line wrapping. See https://github.com/easybuilders/easybuild-easyconfigs/pull/21502/files#r1782351340

@Flamefire Flamefire deleted the codestyle branch October 1, 2024 11:39
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