Change document metadata created, updated to be optional
#753
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
abdulmth
added
Wasm
abdulmth
force-pushed
the
chore/make-created-and-updated-optional
branch
from
6a2fd8c
to
d3c80ea
Compare
added 5 commits
March 28, 2022 17:39
…-updated-optional # Conflicts: # examples/low-level-api/diff_chain.rs # examples/low-level-api/resolve_history.rs
cycraig
reviewed
Mar 28, 2022
There was a problem hiding this comment.
Looks good but a unit test or two for the null -> None deserialization behaviour would be appreciated in diff_iota_document_metadata, since that could easily break in the future with no-one noticing.
Edit: PR changelog title suggestion
Change document metadata `created`, `updated` to be optional
Comment on lines
24
to
35
| #[serde( | ||
| skip_serializing_if = "Option::is_none", | ||
| deserialize_with = "double_option", | ||
| default = "Default::default" | ||
| )] | ||
| created: Option<DiffOption<Timestamp>>, | ||
| #[serde( | ||
| skip_serializing_if = "Option::is_none", | ||
| deserialize_with = "double_option", | ||
| default = "Default::default" | ||
| )] | ||
| updated: Option<DiffOption<Timestamp>>, |
There was a problem hiding this comment.
Nit: Default::default is redundant for two reasons: serde(default) calls Default::default if unspecified, and Option is a special case in serde which defaults to None regardless.
Edit: updated to just omit Default::default rather than entirely.
Suggested change
| #[serde( | |
| skip_serializing_if = "Option::is_none", | |
| deserialize_with = "double_option", | |
| default = "Default::default" | |
| )] | |
| created: Option<DiffOption<Timestamp>>, | |
| #[serde( | |
| skip_serializing_if = "Option::is_none", | |
| deserialize_with = "double_option", | |
| default = "Default::default" | |
| )] | |
| updated: Option<DiffOption<Timestamp>>, | |
| #[serde( | |
| default, | |
| deserialize_with = "deserialize_double_option_diif", | |
| skip_serializing_if = "Option::is_none", | |
| )] | |
| created: Option<DiffOption<Timestamp>>, | |
| #[serde( | |
| default, | |
| deserialize_with = "deserialize_double_option_diif", | |
| skip_serializing_if = "Option::is_none", | |
| )] | |
| updated: Option<DiffOption<Timestamp>>, |
There was a problem hiding this comment.
I stand corrected. default is required here (likely because of deserialize_with?) but the Default::default can be omitted.
There was a problem hiding this comment.
Exactly, without default a diff message without an update to both created and updated will not deserialize correctly. I added a unit test for this case.
Comment on lines
42
to
43
| // Workaround for deserialize 'null' as `Some(DiffOption::None)` instead of `None`. | ||
| fn double_option<'de, T: Diff, D>(de: D) -> Result<Option<DiffOption<T>>, D::Error> |
There was a problem hiding this comment.
Nit: name and comment.
Suggested change
| // Workaround for deserialize 'null' as `Some(DiffOption::None)` instead of `None`. | |
| fn double_option<'de, T: Diff, D>(de: D) -> Result<Option<DiffOption<T>>, D::Error> | |
| // Workaround for deserializing 'null' as `Some(DiffOption::None)` instead of `None`, | |
| // otherwise it's impossible to unset the option. Only works if `Option::None` is | |
| // skipped during serialization. | |
| fn deserialize_double_option_diif<'de, T: Diff, D>(de: D) -> Result<Option<DiffOption<T>>, D::Error> |
abdulmth
changed the title
created, updated optionalcreated, updated to be optional
PhilippGackstatter
approved these changes
Mar 29, 2022
cycraig
approved these changes
Mar 29, 2022
Co-authored-by: Craig Bester <craig.bester@iota.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of change
Make the created and updated timestamps in the DID Document Metadata optional fields.
Links to any relevant issues
fixes issue #731
Type of change
Add an
xto the boxes that are relevant to your changes.