-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Implement JSON.Encoder for Calendar types #14061
Conversation
Really pleased to see this. It was the only barrier to a clean transition from Jason for my benchmarking purposes. |
Can you manually add it to your benches for now and let us know the numbers? :) |
Sure thing. After applying the patch the test suites all pass, and here's the benchmark: args = %{
a: 1,
b: "value",
c: true,
d: "baz",
e: [1],
f: %{f1: 123},
g: [%{g1: "bat"}],
h: "abc-123",
i: ["abc-123"],
j: ["foo", "bar"],
k: [:a, :b],
l: Date.utc_today(),
m: Time.utc_now(),
n: DateTime.utc_now(),
o: NaiveDateTime.utc_now()
}
recode = fn encoder ->
args
|> encoder.encode!()
|> encoder.decode!()
end
Benchee.run(%{"Encode/Decode" => recode}, inputs: Map.new([Jason, JSON], &{&1, &1})) And here are the results:
It's a great improvement for a synthetic, small, but type exhaustive benchmark. |
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.
LGTM!
💚 💙 💜 💛 ❤️ |
@@ -150,6 +150,12 @@ defimpl JSON.Encoder, for: Map do | |||
end | |||
end | |||
|
|||
defimpl JSON.Encoder, for: [Date, Time, NaiveDateTime, DateTime] do |
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.
Should Duration
also be included?
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 asked myself the same and, honestly, I am not sure. We could easily use the ISO representation, but I am not sure how widespread it is, it is definitely less common than the datetime ones.
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.
AFAIK, in the JS world it is common to serialise duration as ISO strings when parsing it to JSON (see Temporal.Duration.toJSON
or dayjs.duration
). As the string format chose in serialization is just a convention, I hopped ISO strings was a good enough format to be used as default.
Should be backported if accepted.