Change Time::DayOfWeek to ISO ordinal numbering based on Monday = 1 #6555
Conversation
src/time.cr
Outdated
| # This method also accepts `0` to identify `Sunday` in order to be compliant | ||
| # with the `Sunday = 0` numbering. All other days are equal in both formats. | ||
| def self.from_value(value : Int32) | ||
| value = (value + 6) % 7 + 1 |
There was a problem hiding this comment.
surely return Sunday if value == 0 would be better here?
There was a problem hiding this comment.
What should happen when the value is greater than 7?
The current implementation pass for the test below, is it the expected behavior?
Time::DayOfWeek.from_value(8).should eq Time::DayOfWeek::Monday
Time::DayOfWeek.from_value(9).should eq Time::DayOfWeek::Tuesday
# and goes on...There was a problem hiding this comment.
I think either makes some kind of sense. But we can make it more strict, no problem.
src/time.cr
Outdated
| # This method also accepts `0` to identify `Sunday` in order to be compliant | ||
| # with the `Sunday = 0` numbering. All other days are equal in both formats. | ||
| def self.from_value(value : Int32) | ||
| value = (value + 6) % 7 + 1 |
There was a problem hiding this comment.
What should happen when the value is greater than 7?
The current implementation pass for the test below, is it the expected behavior?
Time::DayOfWeek.from_value(8).should eq Time::DayOfWeek::Monday
Time::DayOfWeek.from_value(9).should eq Time::DayOfWeek::Tuesday
# and goes on...
|
What's the reason it works for 0? I would just leave the default Enum implementation. |
That why there should be a way to retrieve the week day from a 0-numbered index. As expressed in #6554, this doesn't need to be an override of |
straight-shoota
force-pushed
the
jm/feature/time-day_of_week-iso
branch
3 times, most recently
from
1d555a1 to
c9ed04d
Compare
|
@asterite Sounds fair. |
straight-shoota
force-pushed
the
jm/feature/time-day_of_week-iso
branch
from
c9ed04d to
d46294c
Compare
Fixes #6554
DayOfWeek.from_valuealso identifies0asSundaymaking it work with both numbering schemes. I don't think this should pose a problem.DayOfWeek.newstill requires the proper value, meaning0is not recognized.I did not add a method to retrieve the value based on sunday = 0 (initially proposed as
#value_sun0) because I think it is much easier and maybe even better understandable to just usevalue % 7.