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

Switch to GitHub Actions #22

Merged
merged 1 commit into from
May 20, 2024
Merged

Switch to GitHub Actions #22

merged 1 commit into from
May 20, 2024

Conversation

michaelmior
Copy link
Contributor

This removes the Travis CI configuration and runs tests on GitHub Actions instead. For now I've just removed Codecov, but this at least gets the tests running again. Also supercedes #18.

@michaelmior
Copy link
Contributor Author

Strange. Tests on Python 3.7 passed on my own fork.

@shinnar
Copy link
Member

shinnar commented May 17, 2024

Thanks so much for this!

Two comments:

  • I would suggest just removing python 3.7 support (so removing it from the matrix), as Python 3.7 was EOL in 2023.
  • The package currently specifies its dependencies in setup.py. Why does this PR introduce a requirements.txt file as well? If the requirements.txt file is desirable for a particular use case, then it should be read and used in setup.py as well. I am not against having it, I would just prefer to reduce redundant dependency specifications that can easily get out of date.

@michaelmior
Copy link
Contributor Author

Agreed that Python 3.7 should just be removed. I would also suggest adding 3.11 and maybe 3.12. requirements.txt was added because it's necessary for caching support in GitHub Actions. Agreed that the duplication is bad and reading that in setup.py seems like a good solution.

@michaelmior
Copy link
Contributor Author

I will say however that the two are intended for different purposes and arguably should be kept separate.

@michaelmior
Copy link
Contributor Author

Also, apparently specifying a single . in the requirements.txt will cause it to read from setup.py, which I think would be a better alternative.

Signed-off-by: Michael Mior <[email protected]>
@michaelmior
Copy link
Contributor Author

Okay, I've updated the PR to test against Python 3.8-3.12 and removed the duplication in requirements.txt.

@shinnar shinnar merged commit 14189b8 into IBM:master May 20, 2024
6 checks passed
@shinnar
Copy link
Member

shinnar commented May 20, 2024

Merged!
Thanks!

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