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

Make Python version an input in check-manifest and linting action #71

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sfmig
Copy link
Contributor

@sfmig sfmig commented Nov 28, 2024

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?
Right now, the linting and check-manifest actions are set to run on Python 3.10. It is a bit annoying to manually change this here as we move through supported versions.

With this PR, the Python version for these two actions becomes an input variable with a default value. The default is set to 3.x, which means the latest stable version of Python 3. This seems more convenient and is also more consistent with the build_sphinx_docs action and the test actions.

What does this PR do?
It sets the python-version variable as an input for the lint and check_manifest actions.

References

This came about from a discussion in movement regarding what does it mean when we say we support a specific Python version.

Right now, when we say we support a specific Python version (say 3.12), in practice we mean we run the code tests in Python 3.12. But we don't check in CI if the developer tools (linting and check manifest) also run on Python 3.12 (because it is manually set to 3.10 and we cannot pass it as an input).

More generally, should we treat the developer tools we test on CI the same way as we treat tests? That is, should we ensure that the CI actions related to precommits run all the supported Python versions? @niksirbi mentions in movement for the development enviroment we recommend the middle one of the three supported Python versions at any given point. Is this a widespread convention?

How has this PR been tested?

\

Is this a breaking change?

If we want backwards compatibility with the current situation, we could make the default value 3.10.

Does this PR require an update to the documentation?

\

Checklist:

  • [ n/a ] The code has been tested locally
  • [ n/a ] Tests have been added to cover all new functionality
  • [ n/a ] The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

@sfmig sfmig marked this pull request as ready for review November 28, 2024 13:59
@sfmig sfmig requested a review from a team November 28, 2024 13:59
Copy link
Member

@niksirbi niksirbi left a comment

Choose a reason for hiding this comment

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

I'm happy for this to go through as is.

@niksirbi mentions in movement for the development environment we recommend the middle one of the three supported Python versions at any given point. Is this a widespread convention?

To clarify, this is a convention I just arbitrarily came up with, I don't think it's widespread thing. Perhaps developing on the latest supported version for each package makes more sense?

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