-
Notifications
You must be signed in to change notification settings - Fork 10
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
fix: Handling datetime in client to_dict() #1314
base: main
Are you sure you want to change the base?
Conversation
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 converts datetime objects (which imnsho are correct) to strings. Why do we need to do this?
59dc3c7
to
5a02e28
Compare
def to_dict(self) -> Dict[str, Any]: | ||
"""Return a dictionary representation of this object's attributes""" | ||
return {k: getattr(self, k) for k in self._get_scalar_fields()} | ||
|
||
def to_json_dict(self) -> Dict[str, Any]: | ||
"""Return a JSON encoder friendly representation of this object's attributes""" | ||
return {key: self._serialize(val) for key, val in self.to_dict().items()} |
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.
Python's json
module already supplies a way to convert python values to json-compatible values (docs here and example implementation here ). Do we need to implement this functionality in our client, or can we leave it to users to work with python's standard library?
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.
cc @uermel
Calling
to_dict()
method raises serialization errors for objects that have a date time object. Ex: Dataset, Tomogram,..This fix serializes the date time into the iso_format string.