-
Notifications
You must be signed in to change notification settings - Fork 79
python/tskit/util.py: Add a file name to FileFormatError #3366
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3366 +/- ##
==========================================
+ Coverage 89.76% 90.33% +0.56%
==========================================
Files 29 28 -1
Lines 31292 25694 -5598
Branches 5738 4742 -996
==========================================
- Hits 28089 23210 -4879
+ Misses 1794 1367 -427
+ Partials 1409 1117 -292
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
428857e to
dbf9835
Compare
jeromekelleher
left a comment
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.
LGTM. Can we add a unit test please, checking that the filename goes into the exception as expected, and validating what happens when we use something that doesn't have a name? (Although, it's not obvious to me how you could have a seekable stream that doesn't have a name, but we need to do due diligence here.)
You can pytest.skip for Python < 310.
@benjeffery shall we just ditch Python 3.10 altogether?
Thanks for taking a look, I've trying modifying the existing tests in test_utils.py. I could use some pointers on how to test the stream side of things, where should it go and how do you generally do this? |
|
I think we're going to drop 3.10 ASAP, so let's not worry about the failures on 3.10 CI (and we can remove the conditional at the top). The tests you have look good, updating the existing ones here is fine (although may not need to check the filename on every downstream check for FileFormatError (as that's not what most of the tests are about). Have a look at tests/test_fileobj.py for what happens in the stream case and see what kind of error messages we get. |
Closes tskit-dev#2467 Uses the .add_note method of exception, introduced in Python 3.11. Also hardcodes the file name in the raised from exceptions for HDF5 or zip. Until 3.10 is EOF, needs the sys module.
|
Thanks, reduced the number of tests of FileFormatError. In the TestBadStream, the error_text for matching does not seem to be used as modifying it to something that shouldn't be in the error does not fail the test. For instance: Still passes |
Closes #2467
Uses the .add_note method of exception, introduced in Python 3.11. Also hardcodes the file name in
the raised from exceptions for HDF5 or zip.
Until 3.10 is EOF, needs the sys module.
Description
Would fix #2467
I'll tidy up (changelog and linting) if you think this change is ok.
PR Checklist: