-
Notifications
You must be signed in to change notification settings - Fork 5
feat/allow extra keys in metadata #141
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
Conversation
|
Updated this PR to only require that extra fields be objects with a |
…, and make some handy test fixtures
| RawFillValue = tuple[int, ...] | ||
|
|
||
| FillValue = BoolFillValue | IntFillValue | FloatFillValue | ComplexFillValue | RawFillValue | str | ||
| FillValue = object |
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.
for visibility @dstansby -- we will need to handle fill value validation on a per-datatype basis, because nearly anything can be a valid fill value for some data type. I made this change after expanding some tests and getting pydantic validation errors for some fill values for particular data types supported by zarr-python
| class AllowedExtraField(TypedDict): | ||
| """ | ||
| The type of additional fields that may be added to Zarr V3 Array or Group metadata documents. | ||
| """ | ||
|
|
||
| must_understand: Literal[False] |
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.
this is weaker than what the spec currently requires, because the spec had a regression that hopefully gets fixed soon.
| Extra fields must be dicts with a 'must_understand' key set to False. | ||
| """ | ||
| extra_checker.validate_python(self.__pydantic_extra__) |
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.
the recommended way to get type-checking for extra fields is to give the __pydantic_extra__ field an annotation in the class definition, but this broke due to a NameError during with type annotation resolution. chalking that up as a pydantic bug.
This PR allows the experimental V3 arrayspec and groupspec models to be constructed with extra keys, like
consolidated_metadata. The requirement from the v3 spec is that arbitrary extra fields are permitted as long as they are named + config JSON objects objects with a "must_understand" field set toFalse.