Skip to content
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

Improve image-labels metadata #63

Merged
merged 8 commits into from
Dec 1, 2024
Merged

Improve image-labels metadata #63

merged 8 commits into from
Dec 1, 2024

Conversation

dstansby
Copy link
Contributor

@dstansby dstansby commented Dec 1, 2024

Work towards #36. Not quite finished, but I put everything still left to do in a TODO: comment in the code so it should be easy to tick them off later.

@@ -71,6 +72,11 @@ class ImageAttrs(Base):
min_length=1,
)
omero: Omero | None = None
image_labels: Annotated[ImageLabel | None, Field(..., alias="image-label")] = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we allowed by the spec to write this to JSON as null if the field is unset?

Copy link
Contributor Author

@dstansby dstansby Dec 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not. We can use the exclude_unset keyword argument to model_dump() to make sure we're not dumping any unset, but I think that still leaves open the option that someone sets the value to None then writes 😢 looking at https://docs.pydantic.dev/latest/migration/#required-optional-and-nullable-fields I'm not sure there's a way round this though - perhaps we should just document it as a known issue and advise folks not to set values to None when using this library?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

over in pydantic-ome-ngff I used a special attribute to track the fields that shouldn't be serialized if they are null. we might want to do something similar. But until then I would suggest we keep this as-is and put in a TODO statement about dealing with the null value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I've put in a "known issue" note on the homepage of the docs, definiltey up for us fixing this in a follow up though

@d-v-b
Copy link
Contributor

d-v-b commented Dec 1, 2024

looks good, I have 2 questions that I left as in-line comments

@dstansby dstansby merged commit 68b6cd3 into main Dec 1, 2024
2 of 3 checks passed
@dstansby dstansby deleted the image-labels branch December 1, 2024 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants