-
Notifications
You must be signed in to change notification settings - Fork 2
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
IFSBench: Install via pyproject.toml, absolute import paths and internalised entry points #14
Conversation
3929788
to
325e222
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14 +/- ##
==========================================
- Coverage 78.69% 76.09% -2.60%
==========================================
Files 24 27 +3
Lines 1985 2121 +136
==========================================
+ Hits 1562 1614 +52
- Misses 423 507 +84 ☔ 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.
Looks good so far - I've run some additional checks:
- CMake based install
- Pip install with optional packages
- Use
ifs-bench.py
scripts.
I haven't checked ifs-test
explicitly as this uses tagged versions of ifsbench (i.e. changes here shouldn't affect it) and other upcoming changes (#12 for example) will break ifs-test
in any case.
No issues so far so I am happy to get this merged.
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.
Very nice, thanks for taking care of this!
Just a little pointer on two packages that are no longer required - not strictly related to this PR but it's a good enough opportunity to take care of this maybe?
pyproject.toml
Outdated
tests = [ | ||
"pytest", | ||
"pytest-cov", | ||
"coverage2clover", |
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.
This was just required to convert pytest output to XML for Bamboo to understand. I suggest we remove this now.
pyproject.toml
Outdated
"pytest-cov", | ||
"coverage2clover", | ||
"pylint", | ||
"pytest-pylint", |
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.
Similarly, this was required because it was the best way to route the pylint output via pytest to Bamboo. We're running this separately now, so no longer required.
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.
Thanks, all good from my side. We do have implicit coverage reduction because the CLI scripts are now counted against code coverage (but not tested).
This is a housekeeping PR that moves the installation meta information for pip to
pyproject.toml
, changes to a complete use of absolute imports throughout the package. With that in place, I've then also created a sub-packagecli
and moved the utilities, as well as the external entry point scripts into this internal sub-package. This then allows us to get rid of the__main__
dunder calls and explicit shebangs.Integration note: Please check that the external-facing tools works as you'd expect and that the ifs-test CLI interfacing still works as one expects.