Fix 3303 refactor zoneHelper into getRubyTimeZoneName #3331

Merged
merged 2 commits into from Nov 14, 2015

Conversation

Projects
None yet
4 participants
@tdaitx
Contributor

tdaitx commented Sep 15, 2015

In order to fix #3303 and as requested in #3309 I have extracted the zone name logic into its own method (RubyTime.getRubyTimeZoneName) and reused it in RubyDateFormatter.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Sep 16, 2015

Member

this broke truffle using the zoneHelper method (likely why its been public) and thus the whole build is failing on a compilation error. either keep the original method signature around (mark it a deprecated) so that they can adjust https://github.com/jruby/jruby/blame/master/truffle/src/main/java/org/jruby/truffle/nodes/rubinius/TimePrimitiveNodes.java#L182 or we should ask them for advise // cc @chrisseaton

Member

kares commented Sep 16, 2015

this broke truffle using the zoneHelper method (likely why its been public) and thus the whole build is failing on a compilation error. either keep the original method signature around (mark it a deprecated) so that they can adjust https://github.com/jruby/jruby/blame/master/truffle/src/main/java/org/jruby/truffle/nodes/rubinius/TimePrimitiveNodes.java#L182 or we should ask them for advise // cc @chrisseaton

tdaitx added some commits Sep 15, 2015

refactor zoneHelper to better expose tz name logic
In order to reuse and mimic Time#zone behavior the timezone name
logic in RubyTime.zoneHelper needs to be refactored into 2 separated
methods: one that receives the environment TZ as a String and another
that will extract the enviroment TZ from the runtime.

The isTzRelative is only used by RubyTime itself and there is no
need to expose it, thus it was moved back into RubyTime.zone.

Truffle's org.jruby.truffle.noded.TimePrimitiveNodes has been
updated to make use of the new getRubyTimeZoneName method.
get zone from RubyTime.getRubyTimeZoneName (fixes #3303)
According to the spec "%Z" and Time#zone should behave the
same. This fix makes use of RubyTime.getRubyTimeZoneName
to closely mimic RubyTime.zone behavior.

Note that it is still possible to fail the above spec when the
RubyTime object is created with a numeric TZ relative to UTC
(thus setting isTzRelative to true) because there is no way
to reproduce this particular behavior from within
RubyDateFormatter.
@eregon

This comment has been minimized.

Show comment
Hide comment
@eregon

eregon Sep 16, 2015

Member

The new commits looks good for Truffle, thanks!
Allowing to pass the TZ var as a parameter also seems generally useful.

Member

eregon commented Sep 16, 2015

The new commits looks good for Truffle, thanks!
Allowing to pass the TZ var as a parameter also seems generally useful.

@tdaitx

This comment has been minimized.

Show comment
Hide comment
@tdaitx

tdaitx Sep 16, 2015

Contributor

@kares Thanks for catching this, I should have build it before committing the code as Eclipse was unable to properly detect truffle as a java project and explicitly show the compilation failure. Sorry about that.

I updated my code and build it locally to avoid failures this time.

I also made a small update in Truffle to make use of the new method.

Contributor

tdaitx commented Sep 16, 2015

@kares Thanks for catching this, I should have build it before committing the code as Eclipse was unable to properly detect truffle as a java project and explicitly show the compilation failure. Sorry about that.

I updated my code and build it locally to avoid failures this time.

I also made a small update in Truffle to make use of the new method.

@kares kares added this to the JRuby 9.0.2.0 milestone Sep 17, 2015

@kares kares added the core label Sep 17, 2015

@enebo enebo modified the milestones: JRuby 9.0.4.0, JRuby 9.0.2.0 Oct 21, 2015

@enebo enebo modified the milestones: JRuby 9.0.4.0, JRuby 9.0.5.0 Nov 13, 2015

kares added a commit that referenced this pull request Nov 14, 2015

Merge pull request #3331 from tdaitx/fix-3303-refactor-get-rubytime-t…
…z-name

Fix 3303 refactor zoneHelper into getRubyTimeZoneName

@kares kares merged commit 5698862 into jruby:master Nov 14, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment