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

[Issue #22] consider time as local if no offset is given #47

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

robuye
Copy link
Contributor

@robuye robuye commented Jun 18, 2013

it uses ENV['TZ'] or can be passed as an option (global), if something goes wrong (no env var or unknown timezone) it will fallback to UTC. It's not perfect, but i don't have better idea.

Added facets/date dependency.
Also added pry as development dependency, its really helpful.

@@ -46,6 +50,10 @@ def parse(xml)

private

def update_timezone(tz)
tz ? ENV['TZ'] = tz : false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the return value is not used, so i would change to code to not look like it matters:

ENV['TZ'] = tz if tz

@rubiii
Copy link
Contributor

rubiii commented Jun 18, 2013

thanks @robuye!

can this change be backed by a specification? that would be something to put in the changelog for everyone to follow through as this change probably affects a lot of people. also, timezone changes might not break people's tests and they surely don't expect them. there are people that don't review libraries before updating and this could possibly result in very nasty surprises.

what do you think? what do others think?

@robuye
Copy link
Contributor Author

robuye commented Jun 19, 2013

good point about affecting others, I didn't think about it.
As @cschiewek described, ISO8601 specifies that timezone without offset should be considered as local:

(...)Without any further additions, a date and time as written above is assumed to be in some local time zone.

I think it would be good idea to wait some time and possibly get feedback, this is not an urgent fix.

@rubiii
Copy link
Contributor

rubiii commented Jun 19, 2013

good idea. thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants