Closed ocefpaf closed 3 years ago
Merging #64 (6ca9ecf) into master (17053cc) will not change coverage. The diff coverage is
100.00%
.
@@ Coverage Diff @@
## master #64 +/- ##
=======================================
Coverage 97.75% 97.75%
=======================================
Files 10 10
Lines 445 445
Branches 88 88
=======================================
Hits 435 435
Misses 5 5
Partials 5 5
Impacted Files | Coverage Δ | |
---|---|---|
rechunker/api.py | 100.00% <100.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 17053cc...6ca9ecf. Read the comment docs.
I'm going to cc @martindurant on this. It seems unexpected that fsspec.get_mapper()
evaluates to False. Martin, is this expected behavior or an fsspec bug?
EDIT: THANK YOU @ocefpaf for this PR! Depending on Martin's reply we will either fix upstream or more forward with this.
Since it derives from MutableMapping, I think that bool(mapper) is essentially a test for emptiness, like bool(dict). The behaviour is not explicitly defined in fsspec.
Ok great, so let's go forward with this.
@ocefpaf - clearly the situation in #59 was not covered by our tests. Could you manage to add a simple test for this scenario? I believe the right combo is
I would add an additional parameterization here:
...which parameterizes the store arguments (rather than setting them within the test function).
Thanks Felipe! This is a fine solution.
This should fix #59. It was failing b/c a
fsspec.get_mapper()
evaluates toFalse
. I don't know enough aboutfsspec
but I guess that makes sense if a directory is not created yet. Regardless we should always check for nullness instead ofif var: do it
in those cases.