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

Refactor to poetry groups instead of extras #833

Merged
merged 8 commits into from
Mar 11, 2024

Conversation

danielhuppmann
Copy link
Member

@danielhuppmann danielhuppmann commented Mar 9, 2024

Please confirm that this PR has done the following:

  • Tests Added
  • Documentation Added
  • Name of contributors Added to AUTHORS.rst
  • Description in RELEASE_NOTES.md Added

Description of PR

This PR does a few things:

@danielhuppmann danielhuppmann self-assigned this Mar 9, 2024
Copy link

codecov bot commented Mar 9, 2024

Codecov Report

Attention: Patch coverage is 72.72727% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 95.1%. Comparing base (ddbb88e) to head (cf75d2e).
Report is 2 commits behind head on main.

❗ Current head cf75d2e differs from pull request most recent head 4ca5641. Consider uploading reports for the commit 4ca5641 to get more accurate results

Files Patch % Lines
tests/test_data_worldbank.py 50.0% 2 Missing ⚠️
pyam/unfccc.py 0.0% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@danielhuppmann danielhuppmann marked this pull request as ready for review March 9, 2024 09:08
@danielhuppmann
Copy link
Member Author

Please review @glatterf42

Copy link
Collaborator

@glatterf42 glatterf42 left a 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 :)


#------------------------
# 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
Copy link
Collaborator

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


#------------------------
# 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
Copy link
Collaborator

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -43,12 +43,12 @@ jobs:
with:
path: .venv
key: venv-${{ runner.os }}-${{ steps.setup-python.outputs.python-version }}-${{ hashFiles('**/poetry.lock') }}

Copy link
Collaborator

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.

Copy link
Member Author

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
Copy link
Collaborator

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

pyam/worldbank.py Show resolved Hide resolved
@danielhuppmann danielhuppmann merged commit 661136c into IAMconsortium:main Mar 11, 2024
12 checks passed
@danielhuppmann danielhuppmann deleted the poetry/groups branch March 11, 2024 10:10
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