#473 ✓resolved
Betelgeuse

Use Time.zone.now instead of Time.now

Reported by Betelgeuse | July 24th, 2009 @ 03:05 PM | in Hobo 1.0 - Final

Currently the inputs that ship with rapid use Time.now for the default value which means the server clock is used instead of the configured time zone for the application in question. I think it would be better to use Time.zone.now for localized times. If we want to support people not using config.time_zone then we need a fall back as Time.zone does not exist without having migrated to the new time zone support.

http://ryandaigle.com/articles/2008/1/25/what-s-new-in-edge-rails-e...

Comments and changes to this ticket

  • Bryan Larsen

    Bryan Larsen July 24th, 2009 @ 07:10 PM

    If you send a patch, I'll apply it. Otherwise, it probably won't make Hobo 1.0 given the long list of other things we need to do.

  • Betelgeuse

    Betelgeuse July 26th, 2009 @ 08:44 PM

    If you don't support legacy mode, then it's simply a matter of sed -e "s/Time.now/Time.zone.now/" (and some tests would be nice of course) , would you require legacy support too?

  • Bryan Larsen

    Bryan Larsen July 26th, 2009 @ 08:53 PM

    If the user has not defined Time.zone, they should get the same behaviour as before. If they have defined Time.zone, we'll use that. It'll change behaviour on them, but we'll call attention to it in release notes. We're not at 1.0, yet, so changes like this are OK, I think.

  • Juho

    Juho October 12th, 2009 @ 12:28 PM

    Here's a patch me and Betelgeuse cooked up for this. Some unit tests would of course be nice but don't have mysql on this machine which running the existing stuff would seem to require. If they are required, please offer some pointers.

  • Bryan Larsen

    Bryan Larsen October 14th, 2009 @ 03:43 PM

    • State changed from “new” to “resolved”
    • Milestone set to Hobo 1.0 - Final

    You fixed a bug in my integration tests that I didn't realize I had. :) I'm going to b break my rule and integrate your patch before 0.8.9 rather than after.

  • Iain

    Iain October 15th, 2009 @ 04:05 AM

    • Assigned user set to “Bryan Larsen”

    Sorry guys, but this patch seems to be breaking in 0.89 if Time.zone isn't defined. I get the following error:

    NoMethodError: You have a nil object when you didn't expect it!
    The error occurred while evaluating nil.now

    Should the patch have said:

    def current_time
      Time.zone ? Time.zone.now : Time.now
    end
    

    rather than:

    def current_time
      Time.now ? Time.zone.now : Time.now
    end
    
  • Bryan Larsen

    Bryan Larsen October 15th, 2009 @ 06:32 AM

    It's funny how when reviewing code you read what you expect to see rather than what's actually there. I thought I tested this without a time zone present, but I guess I was mistaken. Thanks for catching this, Iain.

  • Betelgeuse

    Betelgeuse October 15th, 2009 @ 09:18 AM

    Heh, that's why tests are a good thing. You can stub Time.zone in order to test this. It should return a time zone of your choosing like
    ActiveSupport::TimeZone['Helsinki']. We do this in our own tests in order to cover this bug but we don't use hobo with time zones disabled so it wasn't covered. Of course even better would be to make it find out something different from the current time zone.

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป

People watching this ticket

Attachments

Pages