improvements to parse_dtype
#3264
Conversation
…more JSON-like inputs, and test for round-trips
github-actions
bot
added
the
needs release notes
…into widen-parse-data-type
github-actions
bot
removed
the
needs release notes
|
cc @TomNicholas |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3264 +/- ##
==========================================
+ Coverage 59.56% 59.60% +0.03%
==========================================
Files 78 78
Lines 8684 8690 +6
==========================================
+ Hits 5173 5180 +7
+ Misses 3511 3510 -1
🚀 New features to boost your workflow:
|
|
d684ada adds a test to ensure that |
There was a problem hiding this comment.
Nice - I like the name change. Having two identical functions in our API seems a bit confusing from a user POV: https://zarr--3264.org.readthedocs.build/en/3264/api/zarr/dtype/index.html#functions. Could you remove parse_data_type from __all__ so it's removed from the docs, but will still be imported and work for backwards compatibility?
| Interpret the input as a ZDType. | ||
| This function wraps ``parse_dtype``. The only difference is the function name. This function may | ||
| be deprecated in a future version of Zarr Python in favor of ``parse_dtype``. | ||
| Parameters | ||
| ---------- | ||
| dtype_spec : ZDTypeLike | ||
| The input to be interpreted as a ZDType. This could be a ZDType, which will be returned | ||
| directly, or a JSON representation of a ZDType, or a native dtype, or a python object that | ||
| can be converted into a native dtype. | ||
| zarr_format : ZarrFormat | ||
| The Zarr format version. | ||
| Returns | ||
| ------- | ||
| ZDType[TBaseDType, TBaseScalar] | ||
| The ZDType corresponding to the input. | ||
| Examples | ||
| -------- | ||
| >>> parse_dtype("int32", zarr_format=2) | ||
| Int32(endianness="little") | ||
| """ |
There was a problem hiding this comment.
I'd say bin the docstirng here to avoid duplication, and just point to parse_dtype.
There was a problem hiding this comment.
I think duplication is fine here. Happy to revisit this if maintaining two docstrings is a maintenance burden, but my preference is for each function to describe what it does, even if another function does the same thing.
| NullTerminatedBytes(length=10) | ||
| >>> parse_data_type({"name": "numpy.datetime64", "configuration": {"unit": "s", "scale_factor": 10}}, zarr_format=3) | ||
| DateTime64(endianness='little', scale_factor=10, unit='s') | ||
| >>> parse_dtype("int32", zarr_format=2) |
There was a problem hiding this comment.
Huh, does this not need an import? not an issue, just a question
There was a problem hiding this comment.
I think it does need an import
Co-authored-by: David Stansby <[email protected]>
parse_dtype.parse_data_typeis kept around but it just wrapsparse_dtype. The reason for this change is naming consistency -- theZDTypemethods already use the "dtype" abbreviation extensively, so it's potentially confusing thatparse_data_typedoes not.parse_dtype(with the exception of "|O", which is ambiguous).closes #3263