Introduce calendar types #4383

Merged
merged 1 commit into from Mar 15, 2016

Projects

None yet

5 participants

@josevalim
Member

Hello everyone,

This pull request is a proposal for unifying Calendar types in the Elixir ecosystem. Today we have a mixture of different libraries, each defining their own types, which makes interoperability between them really complicated.

Currently, the defined structures do not have any function attached to them, but we expect some basic functionality like ISO8601 parsing and formatting as well as conversion to/from Erlang tuples to be added before Elixir v1.3 is released.

This pull request is not a replacement for the existing date/time libraries. We will still rely on the ecosystem for handling date formatting, date parsing, intervals, periods, time zones and so on.

Huge thanks to @lau and @bitwalker for the discussion that lead to this proposal.

FAQ

Q: Will Elixir support multiple calendars?

While the data structure supports pluggable calendar systems, it is likely Elixir will ship with only the Calendar.ISO implementation. However, the system will be designed so libraries can provide other calendars.

Q: Will Elixir support time zones?

Not in the short term. The time zone support will continue to be provided by libraries like timex and calendar.

Q: Time zone or timezones?

We have chosen to spell it as "time zone". This means time zone modules may be spelled as TimeZone and fields as time_zone.

@josevalim josevalim commented on the diff Mar 13, 2016
lib/elixir/lib/calendar.ex
@@ -0,0 +1,87 @@
+defmodule Calendar do
@josevalim
josevalim Mar 13, 2016 Member

@lau this will obviously conflict with the calendar library. Luckily, the Calendar module in Calendar is mostly a facade module and defines only one macro. In any case, I would like to discuss this conflict explicitly. We have a couple options:

  1. We can rename the Calendar module in Elixir to something else.
  2. We can rename the Calendar module in calendar to something else, like Calendar.Types.
  3. We can add the __using__ function currently defined in your Calendar module to this module with a deprecation warning. This means that, when users migrate to Elixir 1.3, if they use Calendar, they will get a proper deprecation warning. We would remove the deprecation by Elixir 1.4.
@josevalim
josevalim Mar 13, 2016 Member

I see your Calendar module is also used as your documentation entry-point but that is easy to customize on ExDoc. :)

@lexmag
lexmag Mar 13, 2016 Member

I'm for 2nd option.

On a side note, I think __using__ in calendar is intrusive since people calling use Calendar when the only thing they need is a single alias Calendar.X most of the times. When I've seen use Calendar for first time I thought some magic is involved.
Same thing with the timex. :bowtie:

@bitwalker
bitwalker Mar 14, 2016 Contributor

@josevalim I think we should rename Calendar to something different, and use Calendar to define the behaviour for implementing another calendar. Unless you prefer it be both the entry point module for all date/time things + the behaviour definition.

@josevalim
josevalim Mar 14, 2016 Member

The idea of having use Calendar would only be temporary to support folks migrating from Calendar. Then later on we can make it whatever we want.

@lau
lau Mar 14, 2016 Contributor
We can rename the Calendar module in Elixir to something else.

We can rename the Calendar module in calendar to something else, like Calendar.Types.

The Calendar module is only used for one macro. The macro is used to alias e.g. Calendar.Date and Calendar.DateTime. This is done mostly in order to call functions like Calendar.Date.diff as just Date.diff.

If Elixir 1.3 defines Date (without namespacing), then it makes no sense to alias Calendar.Date. Thus there is no point in having a macro for aliasing them in the Calendar module nor in a renamed module.

I guess people will just have to use the entire module name, e.g. Calendar.Date.diff.

We can add the __using__ function currently defined in your Calendar module to this module with a deprecation warning. This means that, when users migrate to Elixir 1.3, if they use Calendar, they will get a proper deprecation warning. We would remove the deprecation by Elixir 1.4.

This is a good idea.

@josevalim
josevalim Mar 14, 2016 Member

Thank you, I will add the deprecation warning so you can safely remove it in the future from your library.

@ericmj ericmj commented on the diff Mar 13, 2016
lib/elixir/lib/calendar.ex
+ behaviour in future versions. For the actual date, time
+ and datetime structures, see `Date`, `Time`, `NaiveDateTime`
+ and `DateTime`.
+
+ Note the year, month, day, etc designations are over specified
+ (i.e. an integer instead of 1..12 for months) because different
+ calendars may have a different number of numbers and so on.
+ """
+
+ @type year :: integer
+ @type month :: integer
+ @type day :: integer
+ @type hour :: integer
+ @type minute :: integer
+ @type second :: integer
+ @type microsecond :: integer
@ericmj
ericmj Mar 13, 2016 Member

We can use integer ranges for all of these except year.

@josevalim
josevalim Mar 13, 2016 Member

See the docs above. Different calendars have a different number of months.

@ericmj
ericmj Mar 13, 2016 Member

So only Calendar.ISO is gregorian?

@bitwalker
bitwalker Mar 14, 2016 Contributor

Types for hour, minute, second, and microsecond could all be made non_neg_integer.

@josevalim josevalim and 1 other commented on an outdated diff Mar 13, 2016
lib/elixir/lib/calendar.ex
+ by one hour. This means certain datetimes never occur or
+ may occur more than once.
+ """
+ defstruct year: 1970, month: 1, day: 1, hour: 0, minute: 0, second: 0,
+ microsecond: 0, calendar: Calendar.ISO
+ @type t :: %__MODULE__{year: Calendar.year, month: Calendar.month, day: Calendar.day,
+ calendar: Calendar.calendar, hour: Calendar.hour, minute: Calendar.minute,
+ second: Calendar.second, microsecond: Calendar.microsecond}
+end
+
+defmodule DateTime do
+ @moduledoc """
+ A datetime implementation with a time zone.
+ """
+ defstruct year: 1970, month: 1, day: 1, hour: 0, minute: 0, second: 0,
+ microsecond: 0, calendar: Calendar.ISO, time_zone: nil,
@josevalim
josevalim Mar 13, 2016 Member

@lau @bitwalker should we default the timezone to "Etc/UTC"?

@lau
lau Mar 13, 2016 Contributor

Why not have all default values except calendar: Calendar.ISO be nil? For sure there will be people who will construct a struct and e.g. not set the year. If that happens let's look at the two situations:

  1. nil default: Eventually an error happens. Let it crash. Great.
  2. Default 1970 year: The default silently paves over the input error and some function happily does some expensive million dollar transaction based on the year 1970. Sad face. (I talked about this for about half a minute here at around 22:50 here https://www.youtube.com/watch?v=keUbVvMJeKY&t=22m50s )
@josevalim
josevalim Mar 13, 2016 Member

Sounds good. To be clear: should we also make hour/minute/second nil values instead of 00:00:00?

@lau
lau Mar 13, 2016 Contributor

Yeah.

@bitwalker bitwalker commented on an outdated diff Mar 14, 2016
lib/elixir/lib/calendar.ex
+
+ @typedoc "The timezone abbreviation"
+ @type zone_abbr :: String.t
+
+ @typedoc "The timezone UTC offset in seconds"
+ @type utc_offset :: integer
+
+ @typedoc "The timezone standard offset in seconds (not zero in summer times)"
+ @type std_offset :: integer
+end
+
+defmodule Date do
+ @moduledoc """
+ A date implementation.
+ """
+ defstruct [:year, :month, :day]
@bitwalker
bitwalker Mar 14, 2016 Contributor

A Date should also have a calendar field. Calendars are more about dates than they are about datetimes, though there are often assumptions made about time for the sake of conversion (such as making the assumption that the epoch for all calendars begins on the first day containing noon of that year).

@bitwalker bitwalker commented on the diff Mar 14, 2016
lib/elixir/lib/calendar/iso.ex
@@ -0,0 +1,11 @@
+defmodule Calendar.ISO do
@bitwalker
bitwalker Mar 14, 2016 Contributor

If it's ok with you, I'd like to submit a PR with my multi-calendar implementation I'm working on. It'll probably be a few days before it's ready, but has a full calendar implementation for Gregorian (proleptic) + ISO extensions, Julian (proleptic), and I'm hoping to also have at least Hebrew finished so that we can see how another modern and vastly more complex calendar, fits in to the API. We can't just lean on :calendar for Gregorian, because the basic functions required for conversions between calendars must be built up from a shared fixed date system. I can describe more in detail in the PR.

@josevalim
josevalim Mar 15, 2016 Member

@bitwalker at this point I am almost confident we will ship with a single calendar implementation in Elixir and leave the remaining ones as extension points. So while we wouldn't merge others calendars, your knowledge on supporting multiple calendars will be extremely important.

@josevalim josevalim Introduce calendar types
258ffd4
@josevalim josevalim commented on the diff Mar 15, 2016
lib/elixir/lib/calendar.ex
+ alias Calendar.AmbiguousDateTime
+ alias Calendar.NaiveDateTime
+ alias Calendar.Date
+ alias Calendar.Time
+ alias Calendar.TimeZoneData
+ alias Calendar.TzPeriod
+ alias Calendar.Strftime
+ end
+ end
+end
+
+defmodule Date do
+ @moduledoc """
+ A date implementation.
+ """
+ defstruct [:year, :month, :day, calendar: Calendar.ISO]
@josevalim
josevalim Mar 15, 2016 Member

Calendar field was added as requested.

@josevalim josevalim merged commit 0497024 into master Mar 15, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@josevalim josevalim deleted the jv-calendar branch Mar 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment