Skip to content

Conversation

dkuku
Copy link
Contributor

@dkuku dkuku commented Dec 30, 2024

continues on top of #14130 the changes in iso.ex are in the previous pr.
Main changes are the use of iodata in iso8601 functions for date related modules that are used in json serialization.
It also exposes the Calendar.ISO.microseconds_to_iodata(ms, p) as public because its the same logic as in the iso module that I refactored.

@spec to_iso8601_iodata(Calendar.date(), :extended | :basic) :: iodata
def to_iso8601_iodata(date, format \\ :extended)

def to_iso8601_iodata(%{calendar: Calendar.ISO} = date, format)
Copy link
Member

Choose a reason for hiding this comment

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

Let’s not expose these functions from the date/time/naivedatetime/datetime APIs yet, only use it internally.

@josevalim
Copy link
Member

@dkuku the other PR has been merged. To keep things simpler, let's only change the JSON implementation, not the actual calendar types. This will require a longer conversation about how to deal with Duration.to_iodata in json, so let's skip that in this PR as well. In a nutshell:

  1. Use iodata for JSON in time, date, ndt, and datetime
  2. Use microseconds_to_iodata in duration

@dkuku dkuku force-pushed the dk_use_iodata_in_iso_8601_and_json branch from a4013b5 to 590c8ae Compare December 31, 2024 10:31
@dkuku
Copy link
Contributor Author

dkuku commented Dec 31, 2024

I'm a bit confused about the suggested change to the JSON implementation. When you mentioned "Only change the JSON implementation," did you mean that I should move the logic for generating the ISO 8601 representation as iodata to the JSON.Encoder module, and keep the existing to_iso8601 functions in the calendar types (e.g., Date, Time, NaiveDateTime, DateTime) unchanged? I had this in mind when I wake up but there are multiple function heads depending on the calendar used.

@josevalim
Copy link
Member

I'm a bit confused about the suggested change to the JSON implementation. When you mentioned "Only change the JSON implementation," did you mean that I should move the logic for generating the ISO 8601 representation as iodata to the JSON.Encoder module, and keep the existing to_iso8601 functions in the calendar types (e.g., Date, Time, NaiveDateTime, DateTime) unchanged? I had this in mind when I wake up but there are multiple function heads depending on the calendar used.

Yes, exactly. Add a new clause that, if the calendar type is Calendar.ISO, we use the special path, otherwise we fallback to today's implementation.

@dkuku dkuku force-pushed the dk_use_iodata_in_iso_8601_and_json branch from 590c8ae to dc45abe Compare December 31, 2024 15:39
@dkuku
Copy link
Contributor Author

dkuku commented Dec 31, 2024

I refactored this pr. Only 2 files are changed now.

@dkuku dkuku requested a review from josevalim December 31, 2024 15:45
@josevalim josevalim merged commit a0fb49f into elixir-lang:main Dec 31, 2024
9 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants