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: align declared supported python versions #64

Closed
wants to merge 2 commits into from

Conversation

Silvanoc
Copy link
Contributor

Poetry specifies that only Python versions from 3.9 upwards are supported. But the package metadata (mostly visible over PyPI) was still declaring support for Python 3.7 and 3.8.

This patch fixes it.

@Silvanoc Silvanoc marked this pull request as draft January 30, 2024 09:56
Poetry specifies that only Python versions from 3.9 upwards are
supported. But the package metadata (mostly visible over PyPI) was still
declaring support for Python 3.7 and 3.8.

This patch fixes it.

Signed-off-by: Silvano Cirujano Cuesta <silvano.cirujano-cuesta@siemens.com>
Black 24.1.1 requires a newline after a docstring. This patch adapts
some files to fulfill that requirement.

Signed-off-by: Silvano Cirujano Cuesta <silvano.cirujano-cuesta@siemens.com>
@Silvanoc Silvanoc marked this pull request as ready for review January 30, 2024 10:35
@Silvanoc Silvanoc requested a review from cthoyt January 30, 2024 10:36
@cthoyt
Copy link
Collaborator

cthoyt commented Jan 30, 2024

I am happy to see increased minimum versions! I don't know what the effect is on other linkml infrastructure, though

@Silvanoc
Copy link
Contributor Author

I am happy to see increased minimum versions! I don't know what the effect is on other linkml infrastructure, though

@cthoyt we are already declaring support for Python 3.9 upwards in the pyproject.toml. This PR is only making explicit the implicit removal of support for Python <3.9.

I can nevertheless understand that you prefer not to merge if you don't have the overview about the impact on other components. I'll assign it @pkalita-lbl, since he appears to have a good overview of the different components.

@Silvanoc Silvanoc requested review from pkalita-lbl and removed request for cthoyt January 30, 2024 12:15
@cthoyt
Copy link
Collaborator

cthoyt commented Jan 30, 2024

I'm all for it! However, I am not responsible for this package, so better to have @sierra-moxon take a look at it.

@pkalita-lbl
Copy link
Contributor

pkalita-lbl commented Jan 30, 2024

Yeah I'll absolutely leave it up to @sierra-moxon to make the final calls about this package.

I'll just note that I reviewed linkml/linkml-runtime#286 before I saw this. I guess this package's python: ^3.9 is the reason for the corresponding change in that linkml-runtime PR. Python 3.8 will go end-of-life in October of this year. Ideally linkml-runtime could continue to support it until then, but if we must drop support earlier it's probably not the end of the world.

I'm also just curious about this:

Poetry specifies that only Python versions from 3.9 upwards are supported

The only thing I see in the docs is this statement about installing Python itself:

Poetry requires Python 3.8+.

And that's just to install/run Poetry itself. That shouldn't have any bearing on the dependencies of packages developed using Poetry.

@Silvanoc
Copy link
Contributor Author

Yeah I'll absolutely leave it up to @cthoyt to make the final calls about this package.

I'll just note that I reviewed linkml/linkml-runtime#286 before I saw this. I guess this package's python: ^3.9 is the reason for the corresponding change in that linkml-runtime PR. Python 3.8 will go end-of-life in October of this year. Ideally linkml-runtime could continue to support it until then, but if we must drop support earlier it's probably not the end of the world.

I'm also just curious about this:

Poetry specifies that only Python versions from 3.9 upwards are supported

The only thing I see in the docs is this statement about installing Python itself:

Poetry requires Python 3.8+.

And that's just to install/run Poetry itself. That shouldn't have any bearing on the dependencies of packages developed using Poetry.

I've already explained the reason why I claim, that Prefixmaps doesn't support Python <3.9 as of now! My wording might have been a bit ambiguous. What I meant is that the pyproject.toml configuration of this project already requires Python to be 3.9 or higher. This PR only makes the lack of support for 3.8 consistent on the GitHub Workflows and project metadata.

@Silvanoc
Copy link
Contributor Author

I've built the package from the source code corresponding tag 0.2.0 with poetry build wheel, then I've tried to install the resulting package on a Python 3.8 environment (using pyenv) and this is the error message that I get:
ERROR: Package 'prefixmaps' requires a different Python: 3.8.18 not in '<4.0,>=3.9'.

So my PR is not removing support for Python 3.8. It was already removed the moment this commit was merged into the main branch! 🤷 It is only making the removal consistent.

@Silvanoc
Copy link
Contributor Author

I've built the package from the source code corresponding tag 0.2.0 with poetry build wheel, then I've tried to install the resulting package on a Python 3.8 environment (using pyenv) and this is the error message that I get: ERROR: Package 'prefixmaps' requires a different Python: 3.8.18 not in '<4.0,>=3.9'.

So my PR is not removing support for Python 3.8. It was already removed the moment this commit was merged into the main branch! 🤷 It is only making the removal consistent.

I have to admit though, that I'm puzzled by the fact that the tests on Python 3.8 seem to run successfuly! I don't know if dependency specification applies differently to poetry install than to a build Wheel.

@Silvanoc
Copy link
Contributor Author

Silvanoc commented Feb 9, 2024

PR #66 already fixes support for Python 3.8 and aligns declared supported python versions (kudos to @pkalita-lbl ). Therefore this PR is not needed anymore, closing it.

@Silvanoc Silvanoc closed this Feb 9, 2024
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