chore: handle new zarr dtype API #100
Conversation
|
i hope it's not a terrible experience! |
|
So far, it was not so bad to get the tests to "pass" but now I want to see what we lost/gained in terms of coverage :) |
tests/test_v2.py
Outdated
| ) -> None: | ||
| with config.set( | ||
| { | ||
| "array.v2_default_filters.bytes": [{"id": "vlen-bytes"}], | ||
| "array.v2_default_compressor.bytes": None, | ||
| } | ||
| ): | ||
| store = zarr.storage.LocalStore(tmp_path) | ||
| store = zarr.storage.MemoryStore() |
There was a problem hiding this comment.
So this "passes" in so far as it gets caught by our dtype check for things currently unhandled by rust (normally MemoryStore would error when it hits the rust code). I think there could be some value in removing some of these tests to make it clear what we are testing for. I think this suite was a good start because it told us "do things work or not" which was a concern for a while, but I think the package may be sufficiently hardened and behavior clarified that we could cut away some of these sorts of tests that don't actually pass through the rust code.
ilan-gold
changed the title
|
Also tested against |
Towards handling zarr-developers/zarr-python#2874
I slimmed down the tests that only test for metadata and don't actually go beyond pipeline creation in b9116f5. I kept the tests that don't pass through the rust, but still need redirection (structured data types, vlen strings etc.) because I think
(a) we may in theory support them at some point and
(b) it does touch the pipeline code, if only somewhat minimally. And at the end of the day, we are promising roundtripping with those types, even if it is just a "fallback."
I would also make this round of updates a
0.2release (incl. #103 and #104 in theory as well).