-
Notifications
You must be signed in to change notification settings - Fork 7
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
Feature/eccodes 24 #33
Conversation
df1ca1c
to
e4dfe95
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #33 +/- ##
========================================
Coverage 94.77% 94.77%
========================================
Files 3 3
Lines 134 134
========================================
Hits 127 127
Misses 7 7 ☔ View full report in Codecov by Sentry. |
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.
All looks good. I have one comment, since eccodes is not actually needed for pyfdb, only for the tests, is it worth making it an optional dependency?
The syntax for setup.py would be:
setup(
name="Package-A",
...,
extras_require={
"test": ["eccodes"],
},
)
and the installation command for optional dependencies looks like:
pip install pyfdb[tests]
or pip install -e ".[tests]"
e4dfe95
to
f0fbe8d
Compare
I added the proposed changes to the testing dependencies to the EDIT: Reverted the changes because the build pipeline is installing the default target and therefore not installing eccodes but testing later on. |
Dependencies like eccodes-python are now only needed for testing dependency group.
f0fbe8d
to
aadc9f0
Compare
Switched pyeccodes to eccodes-python.