-
Notifications
You must be signed in to change notification settings - Fork 39
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
Add try
/except
around ROOT import
#259
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #259 +/- ##
==========================================
+ Coverage 87.71% 89.58% +1.86%
==========================================
Files 5 5
Lines 1083 1085 +2
Branches 231 217 -14
==========================================
+ Hits 950 972 +22
+ Misses 93 81 -12
+ Partials 40 32 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@clelange @alexander-held : I made a crude but simple attempt in this PR to address #99 by only importing ROOT functions in the |
This is very nice to see, thanks for taking care of this! |
Hi @GraemeWatt -- thanks for this! I'm wondering, however, if for My main issue with these changes is that while they move some of the ROOT import failures to later and therefore What do you think? |
5228276
to
2e77ee8
Compare
try
/except
around ROOT import
@clelange : Yes, I agree that keeping the top-level ROOT import and adding a |
I think we can just run some of the tests without installing ROOT, i.e. a separate set of tests. However, we can do that in a later PR. There's a conflict for |
6627427
to
e9f0c86
Compare
@clelange : I used a Pytest custom marker to add a marker |
* Print out an error message if ROOT cannot be imported. * Add some explanation at the end of the README.md file. * Correct a typo in the link to reading_scikithep_histogram.ipynb.
* Add a custom marker to test functions requiring ROOT. * Run subset of tests using "pytest -m 'not needs_root'".
71ec038
to
d51570d
Compare
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.
Awesome, thanks, this is a very nice solution!
reading_scikithep_histograms.ipynb
.pytest -m 'not needs_root'
.📚 Documentation preview 📚: https://hepdata-lib--259.org.readthedocs.build/en/259/