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

[json] add datetime support #57063

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

cedvdb
Copy link

@cedvdb cedvdb commented Nov 11, 2024

Fix #56175

toIso and parse were used as the implementation. This seem like a decent choice as Javascript also uses iso as the default serialization method for dates.

Add date

  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:
  • See our contributor guide for general expectations for PRs.
  • Larger or significant changes should be discussed in an issue before creating a PR.
  • Contributions to our repos should follow the Dart style guide and use dart format.

Note that this repository uses Gerrit for code reviews. Your pull request will be automatically converted into a Gerrit CL and a link to the CL written into this PR. The review will happen on Gerrit but you can also push additional commits to this PR to update the code review.

Copy link

Thank you for your contribution! This project uses Gerrit for code reviews. Your pull request has automatically been converted into a code review at:

https://dart-review.googlesource.com/c/sdk/+/394401

Please wait for a developer to review your code review at the above link; you can speed up the review if you sign into Gerrit and manually add a reviewer that has recently worked on the relevant code. See CONTRIBUTING.md to learn how to upload changes to Gerrit directly.

Additional commits pushed to this PR will update both the PR and the corresponding Gerrit CL. After the review is complete on the CL, your reviewer will merge the CL (automatically closing this PR).

@cedvdb cedvdb changed the title add datetime support [json] add datetime support Nov 11, 2024
Copy link

https://dart-review.googlesource.com/c/sdk/+/394401 has been updated with the latest commits from this pull request.

1 similar comment
Copy link

https://dart-review.googlesource.com/c/sdk/+/394401 has been updated with the latest commits from this pull request.

Copy link

https://dart-review.googlesource.com/c/sdk/+/394401 has been updated with the latest commits from this pull request.

1 similar comment
Copy link

https://dart-review.googlesource.com/c/sdk/+/394401 has been updated with the latest commits from this pull request.

@cedvdb cedvdb changed the title [json] add datetime support [Draft][json] add datetime support Nov 11, 2024
Copy link

https://dart-review.googlesource.com/c/sdk/+/394401 has been updated with the latest commits from this pull request.

@cedvdb cedvdb changed the title [Draft][json] add datetime support [json] add datetime support Nov 11, 2024
Copy link

https://dart-review.googlesource.com/c/sdk/+/394401 has been updated with the latest commits from this pull request.

1 similar comment
Copy link

https://dart-review.googlesource.com/c/sdk/+/394401 has been updated with the latest commits from this pull request.

Copy link

https://dart-review.googlesource.com/c/sdk/+/394401 has been updated with the latest commits from this pull request.

2 similar comments
Copy link

https://dart-review.googlesource.com/c/sdk/+/394401 has been updated with the latest commits from this pull request.

Copy link

https://dart-review.googlesource.com/c/sdk/+/394401 has been updated with the latest commits from this pull request.

@@ -16,6 +16,7 @@ void main() {
'intField': 10,
'doubleField': 12.5,
'numField': 11,
'dateTimeField': '2024-11-11T03:42:29.108308',
Copy link
Author

Choose a reason for hiding this comment

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

Don't judge me, I need to fix my sleep schedule :S

Copy link

https://dart-review.googlesource.com/c/sdk/+/394401 has been updated with the latest commits from this pull request.

1 similar comment
Copy link

https://dart-review.googlesource.com/c/sdk/+/394401 has been updated with the latest commits from this pull request.

Copy link

https://dart-review.googlesource.com/c/sdk/+/394401 has been updated with the latest commits from this pull request.

@cedvdb
Copy link
Author

cedvdb commented Nov 11, 2024

@jakemac53

My reply to you on gerrit is marked as draft and I don't know how to publish it so I'm answering here:

JSON itself does not specify how dates should be represented, but JavaScript does (JavaScript Object Notation). To me this authority is compelling enough but to add to that:

  • it's human readable (timestamps aren't)
  • it's a standard
  • it's sortable

Using a number of milliseconds or seconds or nanoseconds has the issue that the program deserializing has to have knowledge of the unit. This is a real issue when using various services and language that each have their unit.

Using a map is not always sortable and uses more bandwidth.

Copy link

https://dart-review.googlesource.com/c/sdk/+/394401 has been updated with the latest commits from this pull request.

1 similar comment
Copy link

https://dart-review.googlesource.com/c/sdk/+/394401 has been updated with the latest commits from this pull request.

Copy link

https://dart-review.googlesource.com/c/sdk/+/394401 has been updated with the latest commits from this pull request.

1 similar comment
Copy link

https://dart-review.googlesource.com/c/sdk/+/394401 has been updated with the latest commits from this pull request.

@alestiago
Copy link

Eventually we probably need to have some sort of system for custom serialization of specified types or fields.

@jakemac53

100%, if we all agree that we should also provide (at some point) support for specifying how DateTime format will be encoded out of the box, then we could contemplate creating an issue to track the effort and be open to external contributions.

@jakemac53
Copy link
Contributor

@cedvdb Can you apply this patch and upload it? https://gist.github.com/jakemac53/fa4bbab983446b036a769c2c2c2a6a91

This adds some additional tests, cleans up the output a bit for some other cases, and fixes nullable dates in collections.

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.

pkg:json - Support DateTime attributes
3 participants