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

Fix 3303 refactor zoneHelper into getRubyTimeZoneName #3331

Merged
merged 2 commits into from
Nov 14, 2015

Conversation

tdaitx
Copy link
Contributor

@tdaitx 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
Copy link
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

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.
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.
@tdaitx tdaitx force-pushed the fix-3303-refactor-get-rubytime-tz-name branch from 4fbb9fc to d838b19 Compare September 16, 2015 15:05
@eregon
Copy link
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
Copy link
Contributor Author

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
…z-name

Fix 3303 refactor zoneHelper into getRubyTimeZoneName
@kares kares merged commit 5698862 into jruby:master Nov 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

strftime_spec.rb SPEC test fails in "returns the timezone with %Z"
4 participants