Skip to content

Update Calendar.day_of_week callback and Date.day_of_week/2 docs #14176

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jan 11, 2025
Merged

Update Calendar.day_of_week callback and Date.day_of_week/2 docs #14176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b91dfbc
Update callback and Date.day_of_week/2 docs
kipcole9 Jan 11, 2025
4e38396
Update lib/elixir/lib/calendar.ex
kipcole9 Jan 11, 2025
36feb9e
Merge branch 'elixir-lang:main' into day_of_week_docs
kipcole9 Jan 11, 2025
3e5bd1a
Update date.ex
josevalim Jan 11, 2025
f22e6d9
Update lib/elixir/lib/calendar/date.ex
josevalim Jan 11, 2025
d22e9c0
Update date.ex
josevalim Jan 11, 2025
47d50c0
Update lib/elixir/lib/calendar/date.ex
josevalim Jan 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions lib/elixir/lib/calendar.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,15 @@ defmodule Calendar do
`starting_on` represents the starting day of the week. All
calendars must support at least the `:default` value. They may
also support other values representing their days of the week.

The value of `day_of_week` is an ordinal number meaning that a
value of `1` is defined to mean "first day of the week". It is
specifically not defined to mean `1` is `Monday`.

It is a requirement that `first_day_of_week` is less than `last_day_of_week`
and that `day_of_week` must be within that range. Therefore it can be said
that `day_of_week in first_day_of_week..last_day_of_week//1` must be
`true` for all values of `day_of_week`.
"""
@callback day_of_week(year, month, day, starting_on :: :default | atom) ::
{day_of_week(), first_day_of_week :: non_neg_integer(),
Expand Down
14 changes: 12 additions & 2 deletions lib/elixir/lib/calendar/date.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -852,7 +852,7 @@ defmodule Date do
end

@doc """
Calculates the day of the week of a given `date`.
Calculates the ordinal day of the week of a given `date`.

Returns the day of the week as an integer. For the ISO 8601
calendar (the default), it is an integer from 1 to 7, where
Expand All @@ -861,10 +861,19 @@ defmodule Date do
An optional `starting_on` value may be supplied, which
configures the weekday the week starts on. The default value
for it is `:default`, which translates to `:monday` for the
built-in ISO calendar. Any other weekday may be given to.
built-in ISO 8601 calendar. Any other weekday may be used for
`starting_on`, in such cases, that weekday will be considered the first
day of the week, and therefore it will be assigned the ordinal number 1.

The other calendars, the value returned is an ordinal day of week.
For example, `1` may mean "first day of the week" and `7` is
defined to mean "seventh day of the week". Custom calendars may
also accept their own variations of the `starting_on` parameter
with their own meaning.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Muy concerns are two:

  1. The language reinforces a largely unnecessary distinction between Calendar.ISO and other calendars. The Calendar behaviour is about unifying behaviour, not distinguishing is it not?
  2. There is now no mention that for any value of starting_on other than :default and :monday, the return is also an ordinal number for the default calendar.

Those two reasons are why I wrote the language in the PR. To try to unify expectations for any calendar, and to then point out the specific outcome for the very specific (and yeah, very very common) case of the default calendar with the default starts_on

I don't think I have anything else to contribute to this.

Copy link
Member

Choose a reason for hiding this comment

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

The language reinforces a largely unnecessary distinction between Calendar.ISO and other calendars. The Calendar behaviour is about unifying behaviour, not distinguishing is it not?

I believe this is the root of our disagreement. The behaviour is to give calendars the flexibility to assign different meaning to numbers, given some restrictions. It seems day_of_week muddies the water for us, so we can try "teleporting" these questions to other functions, such as year_of_era. In that function, we can only make assumptions about what the Calendar.ISO return. Any other assumption or equivalence about what the numbers mean could potentially be wrong for entries such as the Japanese calendar.

Copy link
Member

@josevalim josevalim Jan 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

There is now no mention that for any value of starting_on other than :default and :monday, the return is also an ordinal number for the default calendar.

Except for the :default atom, you are not required to support any other atom. For example, a Japanese calendar may want to support :月曜日 (Monday according to Google), and that would be specific to them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was actually referring to the use of anything other than :default or :monday in Calendar.ISO returns an ordinal number, does it not? For example:

# Returns an ordinal number, that is also the cardinal number
# 3 == Wednesday
iex(1)> Date.day_of_week ~D[2025-01-01]
3
# Returns an ordinal number.
# 2 != Tuesday
iex(2)> Date.day_of_week ~D[2025-01-01], :tuesday
2

Which I think is something worth mentioning.

Copy link
Member

Choose a reason for hiding this comment

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

Right! They all return ordinal numbers except when they accidentally align to be cardinal. In any case, I added a new comment that makes it clearer, does that help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems day_of_week muddies the water for us

Possibly. I think it's more than when you defined the Calendar behaviour you did something incredibly positive by providing a framework people can build on to represent alternative calendars in use. I don't think any other language provides such a solid foundation.

Many of those alternative calendars are still proleptic Gregorian based. The differences come from which month they start on, whether they are month- or week-based, when day upon which the week starts and the definition of what the first week of the year is.

That results in Fiscal Year calendars for different countries, industry specific calendars like that for the National Retail Federation, and calendars for countries which start their weeks on a different day. Even corporation specific calendars in the US where they can define their own financial year with some quite complex rules.

They are, basically, all the same base calendar. The only practical difference between these calendars is (1) week or month based, (2) starting month of year and (3) starting day of week.

  1. is out of scope for Elixir per your earlier (years ago) comments. I have implemented the Calendar behaviour for week-based calendars in ex_cldr_calendars so they can still play nicely alongside month-based calendars - because they are still based upon the proleptic Gregorian calendar. And I have week-based capabilities in the Cldr.Calendar behaviour for month-based calendars.

  2. months are always ordinal in Calendar. Just as day-of-month is ordinal. It would be nice (in my opinion) to also have Date.iso_month_of_year/2 but I would never suggest that unless we had agreement on Date.iso_day_of_week/2. I really think these are the only two that would be relevant to a standard library. And as discussed these aren't happening.

  3. we've done to death, nothing more I have to say.

Hopefully this helps at least explain a little bit about why I try very hard to harmonise outputs from calendars I build with those from Calendar.ISO and have the same semantics - because basically for this very large category or calendars they are indeed the same semantics.

BTW, even for Date.era_of_year/1, which is used in ex_cldr_dates_times to format dates and times in a localised way, the code that deals with era formatting doesn't have to distinguish between Calendar.ISO and Cldr.Calendar.Japanese. The results are different but how to interpret them is not. (I spent weeks researching the Japanese imperial history to validate the eras - but that's for a beer conversation).

Copy link
Member

Choose a reason for hiding this comment

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

I have added some code comments to the examples. Let me know if it helps. However, I'd like to avoid introducing the word cardinal in the docs:

  1. We don't talk about it anywhere else, so we would need to fully introduce it as a concept, which we wouldn't use ourselves

  2. The majority of Elixir users likely have English as a second language, so introducing words like ordinal/cardinal come also with a language barrier cost :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a very fair point José


## Examples

# 2016-10-31 is a Monday and by default Monday is the first day of the week
iex> Date.day_of_week(~D[2016-10-31])
1
iex> Date.day_of_week(~D[2016-11-01])
Expand All @@ -874,6 +883,7 @@ defmodule Date do
iex> Date.day_of_week(~D[-0015-10-30])
3

# 2016-10-31 is a Monday but, as we start the week on Sunday, now it returns 2
iex> Date.day_of_week(~D[2016-10-31], :sunday)
2
iex> Date.day_of_week(~D[2016-11-01], :sunday)
Expand Down