-
Notifications
You must be signed in to change notification settings - Fork 86
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
Add CI tests with minimum supported versions, update dependencies to fix failing test, fix pip install -e .
#402
Conversation
Note we're now getting
Like reported in #386 |
Love it 😍 Thoughts about keeping the mindeps file up-to-date? Maybe we should add a pull request template which mentions this concern as a checklist item? It would be cool if there was tooling around this practice and our CI could check that it is up-to-date, and even update it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks awesome. I'd really like some documentation about how this file should be maintained before merging it in. A comment block at the top, or a mention in CONTRIBUTING.md
, or a PR template with a checklist, or ...?
To at least have a single-source of dependencies, if we were using setuptools, we could do something like this:
I don't know if you can do the same with Poetry. Edit: You cannot. |
For us I think just manually updating is fine. Almost all of the dependencies haven't been touched in over a year. Given that, I'm inclined to not add additional infrastructure that itself requires maintenance. I think a small, lightweight test to make sure things are consistent would be good (though I'd like to handle that as a follow-up). We can always add more automation in the future if we find it useful. For now, I think we should start with the simple thing first.
Yeah, totally agree. I'll add docs somewhere now. |
I agree with this as well. I like the comments in both files, and I don't think it's a burden since we should rarely be updating those. |
Thanks all for the reviews! |
pip install -e .
pip install -e .
pip install -e .
xref #386 (comment)
Closes #386