-
Notifications
You must be signed in to change notification settings - Fork 76
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
fix: remove unused hist import in test_0965 #994
Conversation
import uproot | ||
import skhep_testdata | ||
|
||
hist = pytest.importorskip("hist") |
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.
hist
is not used in these tests, thus it can be simply removed as an import
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.
hist = pytest.importorskip("hist") | |
pytest.importorskip("hist") |
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.
Wait—we do need this import because to_hist
is used in the tests. These would fail if hist is not installed. We do need the importorskip
, but we don't need to assign it to a variable.
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 for catching this!
I'll merge it when the tests pass.
import uproot | ||
import skhep_testdata | ||
|
||
hist = pytest.importorskip("hist") |
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.
hist = pytest.importorskip("hist") | |
pytest.importorskip("hist") |
I removed the import completely. |
Okay, now it's in a good state. The lines where there's a hidden dependency on hist (Uproot would raise
and
Now that the file has
I'll enable "auto-merge (squash)." |
@all-contributors please add @GaetanLepage for test Thanks! |
I've put up a pull request to add @GaetanLepage! 🎉 |
Test 0965 fails:
It seems that the other tests relying on
hist
import it usinghist = pytest.importorskip("hist")
.I did the same in this problematic test.