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 support for older python versions #887

Merged
merged 8 commits into from
Mar 9, 2024

Conversation

baszoetekouw
Copy link
Contributor

This PR replaces #869
It retains the changes needed to support older python versions.

Also, it makes some changes to the CI, notably:

  • re-enable the pylint tests, as this catch some more stuff than ruff on its own. I can remove this part, if you prefer
  • run the tests for all upstream-supported versions of python (3.8+)
  • split out the tests in separate GA steps, and reduce verbosity, so that it's easier to spot which tests fail.

As a side note: because coverage now is checked in the CI, it might make sense to enable a tool like https://codecov.io/, which provides a nice web interface showing where coverage is missing, and also adds an automatic CI check to check that new PRs don't decrease coverage.

@baszoetekouw baszoetekouw mentioned this pull request Mar 9, 2024
.editorconfig Outdated
charset = utf-8
trim_trailing_whitespace = true
insert_final_newline = true
max_line_length = 120
Copy link
Collaborator

@p12tic p12tic Mar 9, 2024

Choose a reason for hiding this comment

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

Let's keep line length at 100, it's what ruff is configured with now.

coverage combine
coverage report
env:
TESTS_DEBUG: 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is needed to spot errors in case tests fail in CI.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The failing tests usually print a nice log in the end. Is that not sufficient?

@@ -21,11 +26,13 @@ jobs:
python -m pip install --upgrade pip
if [ -f requirements.txt ]; then pip install -r requirements.txt; fi
if [ -f test-requirements.txt ]; then pip install -r test-requirements.txt; fi
Copy link
Collaborator

Choose a reason for hiding this comment

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

If pylint is added to CI then it should be added to test-requirements.txt for installation.

@p12tic
Copy link
Collaborator

p12tic commented Mar 9, 2024

In Fix python>3.11 compatibility what was mean is probably ``Fix python<3.11 compatibility`.

Split out the different tests in separate CI steps ... commit message is too long, should be max 72 characters. Now it overflows.

@p12tic
Copy link
Collaborator

p12tic commented Mar 9, 2024

Overall looks good. I stand corrected, can bring back pylint.

@baszoetekouw
Copy link
Contributor Author

Thanks for the feedback! This should fix the outstanding issues.

@p12tic
Copy link
Collaborator

p12tic commented Mar 9, 2024

Match editorconfig max line length with ruff config should be part of
add editorconfig. Aside from that, all is good. Thanks!

Signed-off-by: Bas Zoetekouw <bas.zoetekouw@surf.nl>
Signed-off-by: Bas Zoetekouw <bas.zoetekouw@surf.nl>
Signed-off-by: Bas Zoetekouw <bas.zoetekouw@surf.nl>
Signed-off-by: Bas Zoetekouw <bas.zoetekouw@surf.nl>
Signed-off-by: Bas Zoetekouw <bas.zoetekouw@surf.nl>
Signed-off-by: Bas Zoetekouw <bas.zoetekouw@surf.nl>
Signed-off-by: Bas Zoetekouw <bas.zoetekouw@surf.nl>
As some python module names are not proper english words

Signed-off-by: Bas Zoetekouw <bas.zoetekouw@surf.nl>
@baszoetekouw
Copy link
Contributor Author

done!

@p12tic p12tic merged commit f18c809 into containers:main Mar 9, 2024
8 checks passed
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.

2 participants