-
Notifications
You must be signed in to change notification settings - Fork 21.8k
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
Add #weekend?, #next_weekday, and #prev_weekday methods to Date, Time, and DateTime #18335
Conversation
The reason for prev_day and next_day is that they're more natural when the date you're working on isn't today. It doesn't make sense to talk about day_in_the_far_past.tomorrow. Tomorrow/yesterday are explicitly tied to today as the reference point. |
Otherwise looking good! Although I'd also like to see |
@dhh: I added I'll take a look at adding the |
I'm loving these methods. Back in 3.2.x I wrote scopes that do just this. |
@georgeclaghorn, I'd actually prefer to have them use advance(days: 1) and advance(days: -1). yesterday/tomorrow are just not clear for this purpose. Even if the result is the same, the code is a lot less clear. I'd like same_time on |
If adding |
Cultural/regional implications could also be a concern. In some countries, for example, Friday is a weekend and/or Sunday isn't. Perhaps provide API like Extracting |
We define what workday and weekend means in the docs, so I'm ok with that. Would have to be weekend_day? - not loving that.
|
@dhh:
I kept this PR divided into three separate commits and CHANGELOG entries. I'm happy to squash them if you'd prefer, but they seemed like distinct sets of changes to me. |
Sorry, I was not being clear :). I think #yesterday and #tomorrow still make sense when you're doing operations on stuff like Date#today. So don't think we need to deprecate them, but we do need prev_day and next_day doing essentially the same thing, so when you operate on not-today dates, it doesn't read odd. |
Also, I prefer #on_a_weekend? over #weekend? – a weekend is the whole thing, sat + sun, so to me it doesn't make sense to ask whether a single day is THE weekend. It's on a weekend, but it's not the weekend by itself. All good with different commits 👍 |
No problem. Just to make sure I'm understanding correctly, you want |
Correct :). Because the implementation is already a sort of alias by using advance, and it's just clearer that way imo. |
@dhh: OK, think everything's good now. |
…ime, and DateTime `#on_weekend?` returns true if the receiving date/time falls on a Saturday or Sunday. `#next_weekday` returns a new date/time representing the next day that does not fall on a Saturday or Sunday. `#prev_weekday` returns a new date/time representing the previous day that does not fall on a Saturday or Sunday.
…ow for Date, Time, and DateTime
@dhh: yes, I agree that the weekend is the whole thing sat + sun, but here we just want to know whether |
Add #weekend?, #next_weekday, and #prev_weekday methods to Date, Time, and DateTime
Thanks @georgeclaghorn! @kuldeepaggarwal I don't quite follow that argument. on_weekend is similar to the form of "[object] in? [set]". |
end | ||
|
||
# Returns a new date/time representing the next day. | ||
def next_day |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhh: I think this might be an alias of tomorrow
. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could have been, and @georgeclaghorn initially implemented it as such, but I find that more confusing. Clarity > DRY.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great... 👍
What about is_business_day? and next_business_day |
How would those differ from weekday?
|
I'm not opposed to |
Holidays are not business day. This may be too country specific to be useful, but thought it would be worth a discussion. Could have default holidays that can be modified as necessary. |
Don’t think we need any aliases here.
|
Ah, I see. Yeah, I think that’s very country/company specific. I don’t think that’d be a good fit for core, but could make an interesting plugin.
|
I really love where this is going, I've written tons of scopes and methods that do similar back in 3.2.x. Would love to help.
|
Many countries follow different work week. Assuming Saturday+Sunday could be limiting the scope. |
What about is_monday? etc... |
I agree with @egilburg we need a way to change a #weekend? days from the default [Sat and Sun] to country specific, e.g. in Saudi Arabia weekend now is Friday and Saturday. Perhaps if we can use locale files to do that. |
It's more work to add and set a config than just overwriting this method in your local app.
|
+1 for overwriting in this case On Sun, Jan 11, 2015 at 10:32 AM, David Heinemeier Hansson <
|
API implies Rails supports this as a feature, and will ensure that whatever code is built upon this later will remain working when the API is used. Developers overwriting core extensions means they entering "hacker territory" and are on their own in dealing with whatever consequences bubble up as a result. That being said, I concede it's not currently a pragmatic difference in this particular part of the code. |
A config option would have the same effect as overwriting the method will.
|
I agree with @egilburg. It is kind of natural to expect a core extension to be "complete", especially something that has got to do with date and time (because the rules for these are well defined and understood, as opposed to say, strings). |
FYI we have a lot of this stuff in the ByStar gem (https://github.com/radar/by_star). |
@dhh wdyt about having PS: Would also love to hear your thoughts on |
Fine with adding |
So the question was about using |
Ah, yes. Following prior pattern as well as the concern that this is a method you'd frequently want to type on say a console. |
Per @dhh's request in #18330:
#weekend?
returns true if the receiving date/time falls on a Saturday or Sunday.#next_weekday
returns a new date/time representing the next day that does not fall on a Saturday or Sunday.#prev_weekday
returns a new date/time representing the previous day that does not fall on a Saturday or Sunday.Rails already provides
#yesterday
and#tomorrow
for dates and times, so I didn't bother implementing#prev_day
or#next_day
.