-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Change Time#date to return a Tuple of year, month, day instead of Time #5822
Change Time#date to return a Tuple of year, month, day instead of Time #5822
Conversation
I'm 👍 for this, since it's essentially an alias. Returning a tuple of year month day would be better. |
I re-added the method returning a tuple. That's actually the best solution and at least an improvement for #5389 |
Why bother? Why would a tuple be preferable to a Time instance? Let's just drop the |
Yeah fair enough, people can just do |
This is faster than IMHO it makes sense to have it but I'm also okay with removing the method entirely. At least it should not return a |
@ysbaddaden @RX14 WDYT about the performance implication? I'd suggest it's worth having this method. |
The first thing to do when talking about performance is to benchmark it. |
Fair enough: TIME = Time.now
Benchmark.ips do |bm|
bm.report("Time.date") { TIME.date }
bm.report("{time.year, time.month, time.day}") { {TIME.year, TIME.month, TIME.day} }
end
As expected, |
Damn, Sure, let's go for this then. |
Well, I'm running the specs in a VM on an old laptop. So the overall speed isn't as bad as it looks when running on reasonable hardware. 😀 |
Fair enough: Time.date 28.55M ( 35.02ns) (± 7.36%) fastest
{time.year, time.month, time.day} 9.7M (103.07ns) (± 7.51%) 2.94× slower on my laptop. Still actually 3 times slower. |
I'm fine with renaming the method, though there are several options to consider:
Overall, I'd still prefer |
I think I lean towards |
758c41d
to
003bcbd
Compare
0a18dd5
to
5eaf649
Compare
Ping 🏓 |
Another approval, please? |
Merge? |
It's a breaking change, we have to wait until |
5eaf649
to
c6673ac
Compare
Rebased and ready to ship. |
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.
We need one more rebase here to solve conflicts.
c6673ac
to
0e36fe1
Compare
This is a proposal to remove the method
Time#date
. It does not actually return a date (there is noDate
class), but a date-time at midnight (time-of-day fields set to zero). It's essentially an alias forTime#at_beginning_of_day
.I think this method would be generally useful, but should not return a
Time
instance because it always includes a time-of-day and therefor is not a date which would range from midnight to midnight. There is no way to tell aTime
instance to have no time-of-day.Maybe this method could also be replaced or re-added later (for example returning a tuple
year, month, day
, but the current implementation makes no sense and is an unnecessary alias.