-
Notifications
You must be signed in to change notification settings - Fork 119
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
Refactor to poetry groups instead of extras #833
Refactor to poetry groups instead of extras #833
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #833 +/- ##
=======================================
+ Coverage 95.0% 95.1% +0.1%
=======================================
Files 64 63 -1
Lines 6134 6142 +8
=======================================
+ Hits 5828 5842 +14
+ Misses 306 300 -6 ☔ View full report in Codecov by Sentry. |
Please review @glatterf42 |
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 overall and tests are passing, some minor notes on questions in-line :)
.github/workflows/nightly.yml
Outdated
|
||
#------------------------ | ||
# install root project | ||
#------------------------ | ||
- name: Install library | ||
run: poetry install --no-interaction --extras "optional_io_formats optional_plotting tests tutorials" --only-root | ||
run: poetry install --no-interaction --with dev,optional_io_formats,optional_plotting,tutorials,wbdata |
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.
Is there any specific reason to remove --only-root
?
The idea is that the step Install dependencies
just before this one installs the dependencies if they have not been recovered from a cache. Thus, the only thing left to install in the Install library
step is the project itself. When optional dependencies were defined as extras, they needed to be mentioned here because poetry removes dependencies from unmentioned extras upon poetry install
. This is not true for groups, though, so we should be fine without --with ...
. So this lines should be either
run: poetry install --no-interaction
or:
run: poetry install --no-interaction --only-root
, I think.
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.
Done
.github/workflows/pytest-legacy.yml
Outdated
|
||
#------------------------ | ||
# install root project | ||
#------------------------ | ||
- name: Install library | ||
run: poetry install --no-interaction --extras "optional_io_formats optional_plotting tests tutorials" --only-root | ||
run: poetry install --no-interaction --with dev,optional_io_formats,optional_plotting,tutorials |
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.
Same comment as in nightly workflow.
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.
Done
.github/workflows/build-docs.yml
Outdated
@@ -43,12 +43,12 @@ jobs: | |||
with: | |||
path: .venv | |||
key: venv-${{ runner.os }}-${{ steps.setup-python.outputs.python-version }}-${{ hashFiles('**/poetry.lock') }} | |||
|
|||
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.
These white spaces seem unnecessary, but not too tragic.
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.
Removed the whitespaces
pyam/__init__.py
Outdated
from pyam.run_control import run_control # noqa: F401 | ||
from pyam.statistics import Statistics | ||
from pyam.statistics import Statistics # noqa: F401 |
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 pyproject.toml, we have two lines that read:
[tool.ruff.lint.per-file-ignores]
"__init__.py" = ["F401"]
This should make it unnecessary to manually specify # noqa: F401
in __init__.py
files. Is there any specific reason to write it? A tool like mypy would not read this configuration setting, so requires its own setting, but ruff should not complain. If something like flake8 is still running in the background of your editor, feel free to switch it off since ruff provides the same functionality. If ruff itself is complaining, we should check its docs if the configuration syntax has changed for the version we're using.
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.
Done
Please confirm that this PR has done the following:
Name of contributors Added to AUTHORS.rstDescription in RELEASE_NOTES.md AddedDescription of PR
This PR does a few things:
This as a reaction to Stalled import on Mac OS and Python 3.12 OliverSherouse/wbdata#74