Skip to content
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 next and previous day of week api to ActiveSupport #26600

Merged
merged 1 commit into from May 30, 2017

Conversation

@iguchi1124
Copy link
Contributor

iguchi1124 commented Sep 23, 2016

Summary

I want a next day of week api on ActiveSupport datetime extentions.

So, I implemented next day of week api.

For example,

date = Date.new(2016, 9, 26)

date.monday? # => true
date.next_monday # => date + 1.week
date.next_tuesday # =>  date + 1.day

WDYT?

P.S.

The difference between next_week(:tuesday) and next_tuesday .

date = Date.today.monday
date.next_week(:tuesday) # == date + 8.days
date.next_tuesday # == date + 1.day
@rails-bot
Copy link

rails-bot commented Sep 23, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@iguchi1124 iguchi1124 force-pushed the iguchi1124:feature-next-day-of-week branch 2 times, most recently Sep 23, 2016
@schneems
Copy link
Member

schneems commented Sep 23, 2016

Mostly active support is on lock down. So probably not cc/ @matthewd

@iguchi1124
Copy link
Contributor Author

iguchi1124 commented Sep 23, 2016

Thank you for reviewing !

Hmm...

I think this API has many usecases.

https://github.com/search?q=def+next_day_of_week&type=Code&utf8=%E2%9C%93

@pixeltrix
Copy link
Member

pixeltrix commented Sep 24, 2016

@iguchi1124 this doesn't seem any different that using next_week? e.g:

>> date = Date.new(2016, 9, 26)
=> Mon, 26 Sep 2016 
>> date.next_week(:monday)
=> Mon, 03 Oct 2016 

We already have too many ways of doing things with dates and times that we have to keep fixing because we missed them (e.g. fixing alias of since to in in #26597). So I don't think this is a good idea, sorry.

@pixeltrix pixeltrix closed this Sep 24, 2016
@iguchi1124
Copy link
Contributor Author

iguchi1124 commented Sep 24, 2016

@pixeltrix

next_week is returns next week's day of week, but

this API returns "next day of week".

for example,

  def test_next_day_of_week
    datetime = DateTime.new(2016, 9, 24, 0, 30) # saturday 
    assert_equal datetime.next_monday, datetime.since(2.days)
    assert_equal datetime.next_tuesday, datetime.since(3.days)
    assert_equal datetime.next_wednesday, datetime.since(4.days)
    assert_equal datetime.next_thursday, datetime.since(5.days)
    assert_equal datetime.next_friday, datetime.since(6.days)
    assert_equal datetime.next_saturday, datetime.since(1.week)
    assert_equal datetime.next_sunday, datetime.since(1.day)
  end
@iguchi1124
Copy link
Contributor Author

iguchi1124 commented Sep 24, 2016

@pixeltrix

When I wanted to next tuesday and today is monday,

next_week(:tuesday) returns today + 1.day + 1.week,

but actually I want today + 1.day.

@pixeltrix
Copy link
Member

pixeltrix commented Sep 24, 2016

@iguchi1124 I see the difference but the method names just don't work right. If some one came up to me in the street on a monday and said "I'll see you next tuesday" then I'd take it to mean the tuesday in the following week and not the day afterwards.

However I can see the need for getting the next occurrence of a particular day from an date/time instance so let me have a think about what the API should be.

@pixeltrix pixeltrix reopened this Sep 24, 2016
@iguchi1124 iguchi1124 force-pushed the iguchi1124:feature-next-day-of-week branch 2 times, most recently Sep 24, 2016
@iguchi1124 iguchi1124 changed the title Add next week of day api to activesupport Add next and previous day of week api to ActiveSupport Sep 24, 2016
@iguchi1124
Copy link
Contributor Author

iguchi1124 commented Sep 24, 2016

@pixeltrix

if the Date/Time instance is monday, I think "next_tuesday" of the Date/Time instance is actually means "this tuesday".

In other words, someone asks "When is the next tuesday of September 26th? " (It is monday) , The answer is September 27th. I think.

But, I don't understand completely that english speaker feel from these method names. 💭

@schneems
Copy link
Member

schneems commented Sep 24, 2016

Next Tuesday is ambiguous. It could mean the 27th or a week from the 27th. Some people think the 27th is " this Tuesday" because it is the only Tuesday in this week and others think it is "next Tuesday" because it is literally the next one. Given the confusion over the naming and the current general bloat of Active Support im voting we close this.

@pixeltrix
Copy link
Member

pixeltrix commented Sep 25, 2016

@schneems I agree that names like next_monday are ambiguous so shouldn't be added but there is a gap as far as I can tell with getting the next or previous occurrence of a day of the week. In fact this did ring a bell and I managed to track down where I'd monkey patched Date to get this functionality in an app. This is what I did:

class Date
  def next_occur(day_of_week)
    target = (DAYS_INTO_WEEK.fetch(day_of_week) + 1) % 7
    days_since((target - wday - 1) % 7 + 1)
  end

  def prev_occur(day_of_week)
    target = (DAYS_INTO_WEEK.fetch(day_of_week) + 1) % 7
    days_since(-((wday - target + 6) % 7 + 1))
  end
  alias_method :last_occur, :prev_occur
end

(Tided up a bit for public consumption 😄 )

So while I agree that both the method names and implementation of this PR are not correct, the need for the API exists.

@rafaelfranca @jeremy wdyt?

@nynhex
Copy link

nynhex commented Sep 25, 2016

I also monkey patched this behavior for my own use case but I would agree that the method names are ambiguous and confusing for some but I'd also agree that the API should exist. But perhaps with an easier to read method nomenclature.

@iguchi1124
Copy link
Contributor Author

iguchi1124 commented Sep 25, 2016

Thank you for your reviews and comments !

I understood these naming is too ambiguous.

How about next_coming_tuesday and last_tuesday or next_occur_tuesday and prev_occur_tuesday ? (also other days of week)

@iguchi1124
Copy link
Contributor Author

iguchi1124 commented Sep 25, 2016

or next_occur(:tuesday) and prev_occur(:tuesday) same as using next_week method?

@iguchi1124 iguchi1124 force-pushed the iguchi1124:feature-next-day-of-week branch 4 times, most recently Sep 25, 2016
@iguchi1124
Copy link
Contributor Author

iguchi1124 commented Dec 3, 2016

@rafaelfranca @jeremy What do you think about this PR?

@iguchi1124
Copy link
Contributor Author

iguchi1124 commented May 29, 2017

@dhh what do you think about this PR?

@dhh
Copy link
Member

dhh commented May 29, 2017

I buy #next_occuring(weekday) and #prev_occurring(weekday) (don't like the occur abbreviation) 👍

@iguchi1124
Copy link
Contributor Author

iguchi1124 commented May 29, 2017

Thank you for reviews. I 'll fix this method name soon!

@matthewd
Copy link
Member

matthewd commented May 29, 2017

next_occurring(:tuesday) is definitely nicer than tomorrow.end_of_week(:wednesday), which I've previously used.

@iguchi1124 note that's public next_occurring/prev_occurring methods: no per-day extras.

@iguchi1124 iguchi1124 force-pushed the iguchi1124:feature-next-day-of-week branch 2 times, most recently May 29, 2017
@iguchi1124
Copy link
Contributor Author

iguchi1124 commented May 29, 2017

I fixed this pull request. please review 🙏 @dhh @matthewd

activesupport/lib/active_support/core_ext/date_and_time/calculations.rb Outdated
@@ -348,7 +350,7 @@ def days_span(day)
end

def copy_time_to(other)
other.change(hour: hour, min: min, sec: sec, nsec: try(:nsec))
other.change(hour: hour, min: min, sec: sec, usec: try(:usec))

This comment has been minimized.

Copy link
@iguchi1124

iguchi1124 May 29, 2017

Author Contributor

ooops

@iguchi1124 iguchi1124 force-pushed the iguchi1124:feature-next-day-of-week branch May 29, 2017
@rafaelfranca
Copy link
Member

rafaelfranca commented May 30, 2017

Could you squash your commits?

@rafaelfranca rafaelfranca assigned dhh and unassigned schneems May 30, 2017
@iguchi1124 iguchi1124 force-pushed the iguchi1124:feature-next-day-of-week branch to c1bf9f5 May 30, 2017
@iguchi1124
Copy link
Contributor Author

iguchi1124 commented May 30, 2017

squashed.

@dhh
Copy link
Member

dhh commented May 30, 2017

❤️

@dhh dhh merged commit 3fbe657 into rails:master May 30, 2017
2 checks passed
2 checks passed
codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
tjschuck added a commit to tjschuck/rails that referenced this pull request Nov 28, 2017
These methods were originally added in rails#26600

This includes a couple of refactors to make these methods behave more similarly to other Date/Time extensions added by Active Support:

1. Use `advance` instead of `since` and `ago` to time-travel — this is particularly important to keep the returned instance’s class matching `self`.  Before this change:

    today = Date.today                     # => Tue, 28 Nov 2017
    today.class                            # => Date
    today.next_occurring(:wednesday)       # => Wed, 29 Nov 2017 00:00:00 UTC +00:00
    today.next_occurring(:wednesday).class # => ActiveSupport::TimeWithZone

  After this change, a Date (or Time, or DateTime) instance is properly returned (just like is shown in the new docs).  This is generally how everything else in DateAndTime::Calculations works.

2. Move the tests from the DateTime tests to the DateAndTimeBehavior tests.  The latter location is mixed in to the core_ext tests for _all_ of Date, Time, and DateTime to test the behavior across all of the classes.  The previous location is for testing core_ext functionality added specifically just to DateTime.

3. Better docs!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.