-
Notifications
You must be signed in to change notification settings - Fork 0
Maintenance updates #69
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
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
corviday
left a comment
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.
Looks good to me; I think any questions I might have had you already answered in our discussion on quail.
| "twine>=6.1.0,<7.0.0", | ||
| "xarray>=2025.4.0,<2026.0.0", | ||
| "xclim>=0.57.0,<1.0.0", | ||
| ] |
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.
quail's pyproject.toml had a note about updating optional dependencies to problems further down the line; does that apply here as well, and if so would it be useful to note it here too?
rod-glover
left a comment
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.
Overall looks good. A few questions about tooling, e.g., Black versions.
| - uses: actions/setup-python@v2 | ||
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-python@v5 | ||
| - uses: psf/black@stable |
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.
In other repos, we've decided to a specific version of Black, rather than @stable, because the default formatting changed periodically. Changes caused reformatting to be required not under our control, which was undesirable. Pinning the version seemed the easiest way to control that. Possibly Black defaults have settled, and we can use @stable without too much angst nowadays. But if not, then perhaps we should pin to a specific version. Any thoughts on that?
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.
Update: I see in pyproject.toml that we install black>=25.1.0,<26.0.0, which is more or less a version pin. There is no guarantee that @stable, or for that matter any specific version will continue to conform to that, and at that point the local version of Black will be silently out of sync with this version. Eventually that is going to cause problems. One solution, adopted in several other repos, is to just install the project dev dependencies and run black against the codebase that way.
| strategy: | ||
| matrix: | ||
| python-version: ["3.9", "3.10"] | ||
| python-version: ["3.11"] |
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.
Gonna assume that the team has decided only 3.11 is presently required. But ... ?
| if: github.ref != 'refs/heads/master' | ||
| run: | | ||
| py.test -m "not online and not slow" -v | ||
| poetry run py.test -m "not online and not slow" -v |
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.
pytest, not py.test has been the officially recommended invocation for 9 years now. Let's not use deprecated naming.
| COPY pyproject.toml poetry.lock* install_pkgs.R ./ | ||
| RUN Rscript install_pkgs.R | ||
|
|
||
| # Add path to libR.so to LD_LIBRARY_PATH |
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.
👍
Pending: