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

Modernize Python support #35

Closed
chuckwondo opened this issue Apr 18, 2024 · 2 comments · Fixed by #38
Closed

Modernize Python support #35

chuckwondo opened this issue Apr 18, 2024 · 2 comments · Fixed by #38
Labels
enhancement New feature or request

Comments

@chuckwondo
Copy link
Collaborator

Many versions of Python have been released (and many have become unsupported) since this repo was first created, and there are at least a couple of places where I've noticed either an unspecified minimum required Python version, or code that is meant to accommodate some very old Python versions.

For example, the classifiers block in pyproject.toml does not list any specific Python versions. I suggest this block be updated appropriately, including a minimum no less than Python 3.8. In fact, the GitHub Actions workflows use only 3.10 or later, so these should likely also be enhanced to perform a matrix of tests across all versions of Python this library is intended to work with (perhaps 3.8 through 3.12, with 3.13 to follow soon).

One place in the code attempts to accommodate a very old package structure for urllib functionality that should be simplified/unified. There may be other places that should be modernized, but maybe not. The codebase is relatively small, but I haven't looked for other spots yet.

@frankinspace frankinspace added the enhancement New feature or request label Apr 18, 2024
@frankinspace
Copy link
Collaborator

Yes I agree. Just a few more points to add.

With poetry the minimum python version supported is defined here:
https://github.com/nasa/python_cmr/blob/develop/pyproject.toml#L15
If you tried to pip install with anything less than 3.8 you would get an error. But I agree, with 3.8 EOL set for October this year this library should have the minimum version increased. I'd be on board with supporting 3.10, 11, and 12.

The CI for this library uses 3.10 for build and test:
https://github.com/nasa/python_cmr/blob/develop/.github/workflows/python-app.yml#L18-L22
Also, very much in support of setting up a matrix build for multiple python versions. I have done this before and it is not too challenging (example: https://github.com/podaac/data-subscriber/blob/7c3895d973c0d9840476220aab7dc50ad526b130/.github/workflows/python-app.yml#L13-L31) but I don't have time at this moment to work on that (bring on the PRs! :) )

Also, very supportive of removing code that is no longer necessary as we upgrade the python version.

@chuckwondo
Copy link
Collaborator Author

@frankinspace, thanks for the follow up on the issues I recently created. I fully intend to submit PRs for them all (unless someone beats me to them). I'm going to start with this one since I think it's perhaps the most fundamental one to address.

chuckwondo added a commit to chuckwondo/python_cmr that referenced this issue Apr 19, 2024
- Refactor code accommodating old urllib package structure
- Add CI testing for Python 3.8 through 3.12
- Automate updating GitHub Actions versions
- Fix all flake8 warnings

Fixes nasa#35
frankinspace added a commit that referenced this issue Apr 22, 2024
* Modernize Python support

- Refactor code accommodating old urllib package structure
- Add CI testing for Python 3.8 through 3.12
- Automate updating GitHub Actions versions
- Fix all flake8 warnings

Fixes #35

* Upgrade flake8

* Remove caching of Poetry install

It is unclear why the CI build fails to find `poetry` even after the
step that runs `pipx ensurepath`, particularly because locally running
the workflow via `act` succeeds. Removing the use of a cache for the
poetry installation itself, so that poetry is always freshly installed,
and thus should always be found on the path.

* Add badges to README.md

* Rename unit test workflow to Tests

---------

Co-authored-by: Frank Greguska <89428916+frankinspace@users.noreply.github.com>
This was referenced Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants