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

Change Time#date to return a Tuple of year, month, day instead of Time #5822

Merged

Conversation

@straight-shoota
Copy link
Member

commented Mar 13, 2018

This is a proposal to remove the method Time#date. It does not actually return a date (there is no Date class), but a date-time at midnight (time-of-day fields set to zero). It's essentially an alias for Time#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 a Time 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.

@RX14

This comment has been minimized.

Copy link
Member

commented Mar 13, 2018

I'm 👍 for this, since it's essentially an alias. Returning a tuple of year month day would be better.

@RX14

RX14 approved these changes Mar 13, 2018

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2018

I re-added the method returning a tuple. That's actually the best solution and at least an improvement for #5389

Show resolved Hide resolved src/time.cr

@straight-shoota straight-shoota changed the title Remove Time#date Change Time#date to return a Tuple of year, month, day instead of Time Mar 19, 2018

@ysbaddaden

This comment has been minimized.

Copy link
Member

commented Mar 19, 2018

Why bother? Why would a tuple be preferable to a Time instance? Let's just drop the #date method.

@RX14

This comment has been minimized.

Copy link
Member

commented Mar 19, 2018

Yeah fair enough, people can just do {time.year, time.month, time.day} to get the same thing.

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2018

This is faster than {time.year, time.month, time.day} because the calendar calculations don't need to be repeated (see #5389) and year_month_day_day_year is not publicly accessible. Go has time.Date and Python datetime.timetuple() which do the same thing (return a tuple).

IMHO it makes sense to have it but I'm also okay with removing the method entirely. At least it should not return a Time.

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2018

@ysbaddaden @RX14 WDYT about the performance implication? I'd suggest it's worth having this method.

@RX14

This comment has been minimized.

Copy link
Member

commented Apr 26, 2018

The first thing to do when talking about performance is to benchmark it.

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2018

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
                        Time.date   8.11M (123.33ns) (± 6.54%)  0 B/op        fastest
{time.year, time.month, time.day}   2.58M (387.84ns) (± 8.96%)  0 B/op   3.14× slower

As expected, {time.year, time.month, time.day} is about 3 times slower because it executes the same calendar operations for each date field.

@RX14

This comment has been minimized.

Copy link
Member

commented Apr 26, 2018

Damn, year_month_day_day_year is a slow method...

Sure, let's go for this then.

@RX14

RX14 approved these changes Apr 26, 2018

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2018

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. 😀

@RX14

This comment has been minimized.

Copy link
Member

commented Apr 26, 2018

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.

Show resolved Hide resolved src/time.cr
@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2018

I'm fine with renaming the method, though there are several options to consider:

  • date -> as before but with different return type
  • date_parts
  • date_tuple (analog to Python's timetuple)
  • to_date

Overall, I'd still prefer date over the alternatives. It's short and precise and I don't see a need to add some description of the returned type. See also #5389 for this; I think it would make sense to have date and clock return the date and clock fields, respectively, similar to Go's.

@RX14

This comment has been minimized.

Copy link
Member

commented Jun 9, 2018

I think I lean towards date_parts personally. No named tuple.

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/remove-Time.date branch from 758c41d to 003bcbd Jun 19, 2018

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/remove-Time.date branch 2 times, most recently from 0a18dd5 to 5eaf649 Sep 4, 2018

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Oct 22, 2018

Ping 🏓

@RX14

RX14 approved these changes Oct 22, 2018

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Nov 27, 2018

Another approval, please?

@RX14 RX14 added this to the 0.28.0 milestone Nov 28, 2018

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2018

Merge?

@RX14

This comment has been minimized.

Copy link
Member

commented Dec 7, 2018

It's a breaking change, we have to wait until 0.28.0 is the next release. That's why I milestoned it.

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/remove-Time.date branch from 5eaf649 to c6673ac Feb 10, 2019

@straight-shoota

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2019

Rebased and ready to ship.

@bcardiff
Copy link
Member

left a comment

We need one more rebase here to solve conflicts.

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/remove-Time.date branch from c6673ac to 0e36fe1 Apr 1, 2019

@bcardiff bcardiff merged commit f3bb83c into crystal-lang:master Apr 1, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@straight-shoota straight-shoota deleted the straight-shoota:jm/feature/remove-Time.date branch Apr 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.