-
Notifications
You must be signed in to change notification settings - Fork 25
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
Align accepted filetypes with docstring description #361
Conversation
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! Is the string type tested explicitly anywhere?
In general this function signature is a little messy, especially as we have file types and backends. I think once we generalise this into an entry point we should clean it up.
Also TIFF and GRIB don't currently work |
TIFF doesn't even work for Kerchunk-backed virtualization? Edit, nevermind forgot about #291 😓 |
String type is tested, but a non-string and non-FileType type isn't. I'm working on getting to 100% code coverage for this section now (apologies for not thinking to put this as a draft first) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #361 +/- ##
===========================================
+ Coverage 58.80% 78.90% +20.10%
===========================================
Files 32 32
Lines 1903 1906 +3
===========================================
+ Hits 1119 1504 +385
+ Misses 784 402 -382
|
Not right now, see #291
No worries at all - thank you for taking initiative to improve this! |
I'm confident the test failures are unrelated to this PR but won't have time to investigate more until Thursday or Friday |
(failure is #371) |
The docs state the filetype
Can be one of {‘netCDF3’, ‘netCDF4’, ‘HDF’, ‘TIFF’, ‘GRIB’, ‘FITS’, ‘dmrpp’, ‘zarr_v3’, ‘kerchunk’}"
but in reality few of these arguments actually worked due to case sensitivity.