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.

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

Conversation

kipcole9
Copy link
Contributor

The callback and the function docs are update to reflect that day_of_week is an ordinal value.

to reflect that day_of_week is an ordinal value.
@josevalim
Copy link
Member

Thank you. Today's implementation does not require the first value to be 1, are we ok with adding this restriction? I'd say it is fine because I don't think any calendar would start counting from zero, except maybe for the year field.

Co-authored-by: José Valim <jose.valim@gmail.com>
@kipcole9
Copy link
Contributor Author

I was trying to use language that did not bind the first..last range to any particular domain. I suppose my example of "1 does not mean Monday" could create that impression?

Should I add a note reminding that the spec just says non_neg_integer?

@josevalim
Copy link
Member

@kipcole9 the Calendar.ISO docs were good, I was worried about the Date one, so I pushed my changes cause I think it would be more productive. Are you happy with them? Feel free to add/remove on top.

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

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é

@josevalim
Copy link
Member

@kipcole9, would you like to do other changes before I go ahead and merge? :)

@kipcole9
Copy link
Contributor Author

I think I have said more than enough José. Nothing further to add.

@josevalim
Copy link
Member

Ok, merging. I believe it is not the changes you would have done but I hope it is clear to hear and act on all points brought up.

@josevalim josevalim merged commit c246400 into elixir-lang:main Jan 11, 2025
@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