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

Warn about Rails v Ruby Date/Time libraries #182

Closed
cjhin opened this issue Jun 4, 2016 · 7 comments
Closed

Warn about Rails v Ruby Date/Time libraries #182

cjhin opened this issue Jun 4, 2016 · 7 comments

Comments

@cjhin
Copy link

cjhin commented Jun 4, 2016

tl;dr

Ruby Date/Time and Rails Date/Time handle timezones differently, which can cause weird edge cases:

Timecop.freeze(Time.utc(2016, 1, 1, 1))
=> 2016-01-01 01:00:00 +0000
Date.today
=> 2016-01-01
Date.tomorrow
=> 2016-01-01 # ?????

Sometimes, people (including myself) assume Timecop is broken, and file unnecessary issue requests.
One possible solution is to mention in Timecop docs so people stop blaming Timecop 😋


Details

Ruby has Date, Time, and DateTime libraries.

Rails has monkey patched these libraries with additional methods.
(Rails: Date, Time, and DateTime)

Ruby Date/Time methods use the local system timezone, while Rails Date/Time methods use the timezone defined in Time.zone. When those two timezones don't match for some reason AND a developer accidentally uses methods from both classes together, things can break.

There is a really old issue request against Rails where the developers discuss this edge case.

Example

# Timecop isn't the problem, but it's useful to setup the example.
# Let's use Timecop to freeze to a Date/Time that's easy for mental math
Timecop.freeze(Time.utc(2016, 1, 1, 1))
=> 2016-01-01 01:00:00 +0000 # The +0000 tells us that the local machine is in UTC

# Ruby methods use the local machine time (in this case UTC).
# Everything looks normal so far.
[Date.today, DateTime.now, Time.now].join('  |  ')
=> "2016-01-01  |  2016-01-01T01:00:00+00:00  |  2016-01-01 01:00:00 +0000"

# Rails uses Time.zone, which is usually set in application.rb by `config.time_zone = ...`
# Let's manually set it to make a point.
Time.zone = -2
=> -2

# Uh oh, these results don't match the Ruby ones above.
[Date.current, DateTime.current, Time.current].join('  |  ')
=> "2015-12-31  |  2015-12-31T23:00:00-02:00  |  2015-12-31 23:00:00 -0200"

# What happened?
# Time.zone = -2 caused the local (UTC) and Time.zone (-0200) times 
# to fall on two different days, as 01:00 - 0200 = 23:00 (of the previous day)
Time.now.to_date
=> Fri, 01 Jan 2016
Time.current.to_date
=> Thu, 31 Dec 2015

# The following functions are often accidentally used together.
# Rails `Date.current` is the intended replacement for Ruby `Date.today`.
# Unfortunately this nuance is often missed by developers.
[Date.yesterday, Date.current, Date.today, Date.tomorrow].join('  |  ')
=> "2015-12-30  |  2015-12-31  |  2016-01-01  |  2016-01-01"

Possible Solutions

  • I think it might be nice to add a note in the readme near the "Works with regular Ruby projects, and Ruby on Rails projects" comment. Something like:
    • "Warning, Rails Date/Time methods don't play nice with Ruby Date/Time methods"
    • and/or
    • "Be careful mixing Date.yesterday, Date.today, and Date.tomorrow things might break"

Related Issues

Pretty sure they are related:
#169, #148, #62, #16

Sort of sure they are related:
#136, #100, #41

@yaauie basically describes this problem in his comments on PR #100, but it kind of gets lost in the other discussions on that thread. And by that I mean I failed to notice until right now... 🙈

@vemv
Copy link

vemv commented Oct 27, 2016

Ruby has Date, Time, and DateTime libraries.
Rails has Date, Time, and DateTime libraries too!

Isn't this inaccurate? It's more like, Rails monkey-patches Date/Time/DateTime for making them saner or richer. Right?

@cjhin
Copy link
Author

cjhin commented Oct 27, 2016

Isn't this inaccurate? It's more like, Rails monkey-patches Date/Time/DateTime for making them saner or richer. Right?

@vemv hah I think "saner" may be a debatable point, but good catch! I've edited the original issue description.

Unrelated, does anyone know if this gem is still being maintained in this repo (or at all) anymore?

@ballcheck
Copy link
Contributor

Hi @cjhin I'm doing some maintenance

@cjhin
Copy link
Author

cjhin commented Jan 25, 2017

Hi @ballcheck! Let me know if I can help in any way!

@ballcheck
Copy link
Contributor

ballcheck commented Jan 25, 2017

For anyone who's still confused

Time.zone = -1
#=> -1
Time.now.hour
#=> 6
Time.current.hour
#=> 5

ballcheck added a commit that referenced this issue Apr 21, 2017
@ballcheck
Copy link
Contributor

@cjhin done in d30e19f

closing this issue

@cjhin
Copy link
Author

cjhin commented Apr 21, 2017

@ballcheck thanks so much, it looks great!

Thoughts on closing (what I think are related) issues #169 and #148? I'm happy to leave comments on them and/or close them, let me know what would be most helpful!

breville added a commit to code-dot-org/code-dot-org that referenced this issue Apr 8, 2019
As well as code review feedback, this change unit tests the new priority deadline date field.

It was interesting to discover that the unit tests failed when the local machine had its time zone set to something other than PST, and it turns out to be a known issue discussed at travisjeffery/timecop#182.  The solution is to use `Date.current` rather than `Date.today`.

A bunch of factories were also updated to use the yyyy-mm-dd date format that it turns out we're storing, as described in #27895
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants