Few months ago my PR #3547 was accepted.
Ever since I was thinking about adding week_start option to rails app config.
Last comments on PR #3547 pushed me to come up with this PR.
Here config.week_start option added, beginning_of_week, end_of_week and other methods updated to use Date.week_start as default.
Date.week_start can be set via config or using Date.week_start= for current request\thread (mimics time zone).
Method sunday (monday) updated to always return Sunday (Monday) to avoid possible confusion.
next_week and prev_week updated the same way, but they can be modified to work correctly even if week_start is not :monday.
If feedback to this PR will be positive I can update them along with the API docs.
option renamed to beginning_of_week
I've updated next_week and prev_week methods: now they return date of the day in the next (previous) week with Date.week_start taken into account.
How about to set day parameter default to Date.week_start for those methods, so that next_week returns beginning of next week (prev_week - beginning of previous week)?
Does this PR makes any sense?
I've updated next_week and prev_week to use Date.week_start as a default parameter.
So now those methods return correct start of next (previous) week with respect to week_start default.
It shouldn't break backwards compatibility because default is still :monday.
This pull request doesn't merge properly anymore, and will need a rebase.
@steveklabnik - thanks a lot for pointing!
I've rebased this PR towards the latest master.
Let me know if anything else should be done.
Just to throw a little more fuel on the fire...
Date::DAYS_INTO_WEEK starts with Monday equal to zero. However, Ruby's own Date formatters always return Monday as 1, even if you specify Monday as the start of the week. Changing the formatter only influences whether Sunday should equal 0 or 7 (as required by Date.commercial and other calendar week based methods).
d = DateTime.parse("Sun, 15 Jul 2012 12:00:00 +0000")
# Sunday is the start of the week
# Sunday: 0
# Monday: 1
d.strftime("%w").to_i # %w - Day of the week (Sunday is 0, 0..6)
# Monday is the start of the week
# Sunday: 7
# Monday: 1
d.strftime("%u").to_i # %u - Day of the week (Monday is 1, 1..7)
Furthermore, I18n expects Sunday to be 0.
=> ["Sunday", "Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday"]
IMHO, Date::DAYS_INTO_WEEK needs to go, or at least be reconciled with Ruby's notion of the index of days. It's obviously broken.
@jamezilla you're right there's an obvious inconsistency between rails and ruby in treating week days.
However I don't think that changing Date::DAYS_INTO_WEEK is a good idea (at least for now) - it probably will break a lot of existing code that already rely on it.
Being able to set week start by default seems like a good solution for me but let's see if this PR will be accepted.
Agreed. I think deprecating Date::DAYS_INTO_WEEK is a good start and then maybe killing it in rails 4.0?
I've rebased towards the latest master.
/cc @jeremy would you mind to take a look at this?
This adds to the feeling that Date already knows too much about Week. For the original usage, we really wanted #previous_monday to get the beginning of the work week, not the calendar week. As far as I know, #end_of_week was only added for symmetry.
This PR seems like a natural step, in keeping with the rest of the API, but the added complexity here makes it clear that handling weeks & calendaring in Date needs rethinking too. A new Rails-wide setting, default attr, and thread-local all to track calendar week config, all on Date itself. Begging for a separate Week.
That said, this seems a fine addition for now. I wouldn't know what config.week_start means without reading documentation, so perhaps that should be something clearer, like config.beginning_of_week = :sunday.
config.beginning_of_week = :sunday
@jeremy thanks for looking through!
I've rebased everything towards the latest master, merged commits and renamed option to beginning_of_week that is indeed more clear.
We will need a CHANGELOG entry
API docstrings updated for Date#next_week and Date#prev_week, tests added for Date#monday and Date#sunday as soon as those are no more aliases but a separate methods.
@rafaelfranca changelog updated. I'm not sure am I allowed to update it directly by myself though.
This will need rebased.
@steveklabnik thanks for pointing - that's due to changelog entry. Rebased.
Yeah, CHANGELOGs have a way of doing that. :/
This method should be private
We still need to update the guides as for example the configuring
Date.beginning_of_week thread local and beginning_of_week application…
… config option added (default is Monday)
Guides updated - initializer and config option items added to guides/source/configuring.md
Sure, it should be private - just forgot to move, fixed.
fix typo in `DateAndTime::Calculations#all_week` doc [ci skip]
`Date.week_start` does not exist. `Date.beginning_of_week` seems to be correct.