-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add missing Dataset field serializers #384
Conversation
This depends on #381 |
@aaraney, I've marked the issue as no longer blocked. Probably this just needs the branch rebased. |
@robertbartel, rebased and ready for review! Thanks in advance! |
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.
There's one thing that needs to be cleared up before I approve, although I probably am just missing something about how Pydantic works.
python/lib/core/dmod/core/dataset.py
Outdated
return value | ||
|
||
field_serializers = { | ||
"uuid": str, |
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.
I think uuid
should have something akin to the lambda used for manager_uuid
, although perhaps it's declaration needs adjustment. That is:
uuid: Optional[UUID] = Field(default_factory=uuid4)
If I follow correctly, this implies the possibility of explicitly receiving None
. It's possible there's a subtlety about Pydantic I'm missing that requires this to be declared as Optional even though it really isn't (although if that's the case, it's bothersome).
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.
Great catch. You are right. Changing that now.
Great catch! Thanks for the review, @robertbartel! I just pushed the required change. This is ready for another quick review. |
fixes #377
Changes
dmod.core.dataset.Dataset
field serializers (fixes Add field_serializers for Dataset field without serializable types #377).