Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Time::new should treat numeric offsets as the GMT offset in seconds. #539

Merged
merged 1 commit into from

2 participants

@jvshahid

This patch fixes two failing specs in ruby spec. I don't know what do you think about the way I'm caching the timezone for integer offsets, but I think it's simple enough that I don't care.

@headius
Owner

We'd be mixing key types...but I don't really see that as a problem here, since both string (name of TZ) and integer (numeric TZ offset) wouldn't be ok to have in the same cache.

However, I wonder if the Joda call to get DateTimeZone from a numeric offset is going to return the same zone as MRI would. Investigating a bit before merging.

@headius
Owner

Ok, I confirmed that Joda appears to be using a generic TZ when given a numeric offset, which is the best we can expect:

ext-jruby-local ~/projects/jruby $ jruby -e "puts org.joda.time.DateTimeZone.forID('America/Chicago')"
America/Chicago

ext-jruby-local ~/projects/jruby $ jruby -e "offset = -6; puts org.joda.time.DateTimeZone.forOffsetMillis(offset * 60 * 60 * 1000)"
-06:00

These are both GMT-6, but only one of them has the regional information needed for DST, etc. Of course it's impossible to get the proper regional time zone from just a numeric offset, so your patch is probably as good as we can expect.

For reference: MRI gets around this by not doing anything. It always translates incoming string and numeric offsets into numeric offset and just uses that value. Joda's representation of TZ is much richer, so we're forced to always translate string and numeric offsets into a DateTimeZone. That gives us better regional representation of TZ, but means we can sometimes differ from MRI's behavior.

@headius headius merged commit 069ed00 into jruby:master

1 check passed

Details default The Travis build passed
@jvshahid jvshahid deleted the jvshahid:time-new branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 18, 2013
  1. @jvshahid
This page is out of date. Refresh to see the latest.
Showing with 15 additions and 1 deletion.
  1. +15 −1 src/org/jruby/RubyTime.java
View
16 src/org/jruby/RubyTime.java
@@ -153,7 +153,18 @@ public static DateTimeZone getLocalTimeZone(Ruby runtime) {
return getTimeZone(runtime, tz.toString());
}
}
-
+
+ public static DateTimeZone getTimeZone(Ruby runtime, long seconds) {
+ // append "s" to the offset when looking up the cache
+ String zone = seconds + "s";
+ DateTimeZone cachedZone = runtime.getTimezoneCache().get(zone);
+
+ if (cachedZone != null) return cachedZone;
+ DateTimeZone dtz = DateTimeZone.forOffsetMillis((int) (seconds * 1000));
+ runtime.getTimezoneCache().put(zone, dtz);
+ return dtz;
+ }
+
public static DateTimeZone getTimeZone(Ruby runtime, String zone) {
DateTimeZone cachedZone = runtime.getTimezoneCache().get(zone);
@@ -1148,6 +1159,9 @@ private static RubyTime createTime(IRubyObject recv, IRubyObject[] args, boolean
dtz = DateTimeZone.UTC;
} else if (args.length == 10 && args[9] instanceof RubyString) {
dtz = getTimeZone(runtime, ((RubyString) args[9]).toString());
+ } else if (args.length == 10 && args[9].respondsTo("to_int")) {
+ IRubyObject offsetInt = args[9].callMethod(runtime.getCurrentContext(), "to_int");
+ dtz = getTimeZone(runtime, ((RubyNumeric) offsetInt).getLongValue());
} else {
dtz = getLocalTimeZone(runtime);
}
Something went wrong with that request. Please try again.