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 `before?` and `after?` methods to date and time classes #32185

Merged
merged 1 commit into from Mar 26, 2018

Conversation

Projects
None yet
7 participants
@nholden
Contributor

nholden commented Mar 7, 2018

Equality comparisons between dates and times can take some extra time to comprehend. I tend to think of a date or time as "before" or "after" another date or time, but I naturally read < and > as "less than" and "greater than." This change seeks to make date/time comparisons more human readable.

Add `before?` and `after?` methods to date and time classes
Equality comparisons between dates and times can take some extra time to
comprehend. I tend to think of a date or time as "before" or "after"
another date or time, but I naturally read `<` and `>` as "less than"
and "greater than." This change seeks to make date/time comparisons more
human readable.
@rails-bot

This comment has been minimized.

rails-bot commented Mar 7, 2018

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @kamipo (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.

@nholden

This comment has been minimized.

Contributor

nholden commented Mar 13, 2018

Hi, @kamipo! I'd love to hear your thoughts on this PR. Is there any other context I can provide that would be useful, or do you think we should pull anyone else in? Thanks in advance!

@nholden

This comment has been minimized.

Contributor

nholden commented Mar 19, 2018

Hi, @kaspth! I noticed that you recently reviewed another Active Support PR, would you mind taking a look at this one when you have a chance? Sorry if you're not the best person to ask -- let me know if it would be better for me to reach out to someone else. Thanks!

assert_equal false, Time.utc(2017, 3, 6, 12, 0, 0).before?(Time.utc(2017, 3, 6, 11, 59, 59))
assert_equal false, Time.utc(2017, 3, 6, 12, 0, 0).before?(Time.utc(2017, 3, 6, 12, 0, 0))
assert_equal true, Time.utc(2017, 3, 6, 12, 0, 0).before?(Time.utc(2017, 3, 6, 12, 00, 1))
end

This comment has been minimized.

@kerrizor

kerrizor Mar 26, 2018

Contributor

Instead of testing behavior (since what you're really testing is the underlying implementation of :> and :<

time = Time.now # or whatever instance flavor you're testing
time.method(:before) == time.method(:<)
time.method(:after) == time.method(:>)

This comment has been minimized.

@nholden

nholden Mar 26, 2018

Contributor

Interesting, I hadn't considered tests like that. Even though before? and after? are simply alias methods, I think it's valuable to have tests that look at actual dates and times since we'll always expect them to pass regardless of any changes to < or >. What do you think?

This comment has been minimized.

@rafaelfranca

rafaelfranca Mar 26, 2018

Member

We prefer to test behavior in Rails, not the implementation. If the implementation changes we don't have to change the test. So this test is actually nice.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Mar 26, 2018

Thank you for the pull request but we prefer to avoid adding new method to Active Support unless we have a good reason to do so. Since this method is just an alias to > and < I don't think it is worth.

@nholden

This comment has been minimized.

Contributor

nholden commented Mar 26, 2018

we prefer to avoid adding new method to Active Support unless we have a good reason to do so

Thanks for this context!

Regardless of the implementation, I think it's much easier to understand something like...

time.before? other_time

...than...

time < other_time

...at a glance, especially when dealing with many time comparisons or times that we've already performed some operations on. My argument is only based on my own experience though, so I can see how that might not meet the threshold of providing a "good reason."

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Mar 26, 2018

I'm actually neutral about this one and I can see how it improve readbility. So I'll call our Chief API Designer to give us his opinion.

@dhh what do you think about this one?

@rafaelfranca rafaelfranca reopened this Mar 26, 2018

@dhh

This comment has been minimized.

Member

dhh commented Mar 26, 2018

I like this a lot. It's much more natural to talk about dates in form of before and after than lesser and greater than. Has my approval on the API side 🙏

@rafaelfranca rafaelfranca merged commit 9a9ef96 into rails:master Mar 26, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nholden nholden deleted the nholden:human_readable_date_time_comparisons branch Mar 26, 2018

@Alamoz

This comment has been minimized.

Alamoz commented Mar 26, 2018

I'm surprised this hasn't happened sooner. A good DSL should use verbs that are natural to the normal use cases.

I use Date and Time objects a lot. This will make them nicer to work with.

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Mar 31, 2018

Move implementation of `before?` and `after?` to `DateAndTime::Calcul…
…ations`

This prevents duplication of code.

Prevent duplication of tests by moving them to `DateAndTimeBehavior`.

Related to rails#32185.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment