Full Timezone Support

[dcosson]

This adds full timezone support to ice_cube (when using ActiveSupport). It builds off of (and supercedes) #295

The naming when discussing timezones gets confusing, there are named UTC offsets (such as PDT = UTC - 7 hours and PST = UTC - 8 hours) that are often referred to as timezones. There are also timezones that map to a geographical region and account for daylight savings time there, such as those specified in the IANA Timezone Database like "America/Los_Angeles". These are also referred to as timezones. The latter is much more useful, because you can represent things like 5 hours after midnight on the boundary of a daylight savings time border and get the right value.

Previously the to_ical method with timezones enabled uses "%Z" strftime which outputs timezone offsets like PDT, but it should instead output timezones from the IANA timezone database. This PR updates this.

The code in #295 for from_ical is already expecting the timezone to be of the latter format, so that code works as expected.

[dcosson]

@seejohnrun any chance of getting this merged in?

[seejohnrun]

@dcosson nice and thank you! - what about the case where ActiveSupport isn't around?

[Tybot204]

Would love to see this get merged in! Any chance non-ActiveSupport can be added soon so this can merge?

I'm having a problem right now with a Rails app I have running on Heroku. The system time zone of Heroku machines is set to UTC I believe, which means all of my IceCube::Schedule objects save with the incorrect timezone currently.

[dcosson]

To be honest I've never used ruby without ActiveSupport and my snarky first thought is why would anyone even bother :)

I can try to get this working if that's the blocker though. It looks like the only code path that assumes ActiveSupport timezone is available is when you parse an ical string that has a TZID. Can I just raise an explicit exception in this case? Or is there another way to use TZInfo without ActiveSupport?

[Tybot204]

@dcosson You may be able to use this gem: https://github.com/tzinfo/tzinfo

You could make that a dependency of IceCube and use it for parsing dates using whatever timezone is passed in TZID. It doesn't look like it depends on ActiveSupport. Maybe use ActiveSupport by default and use the gem above as a fallback?

EDIT: Just realized that's the exact gem ActiveSupport uses right now.

[Tybot204]

@dcosson @seejohnrun Put some more thinking / console testing into this and I may have a solution. We can modify the time zone parser to use Time.use_zone instead:

def self._parse_in_tzid(value, tzid)
  time_zone = tzid ? tzid.split('=')[1] : Time.zone
  Time.use_zone time_zone do
    Time.zone.parse(value)
  end
end

This will switch Time to use the time zone of whatever was specified in the TZID of the iCalendar event, falling back to whatever the system / server is configured to be by default. I believe this produces identical results to using your previous:

ActiveSupport::TimeZone.new(time_zone).parse(value)

EDIT: Nevermind, I had ActiveSupport loaded somehow in my Ruby console. use_zone is an extension to Time added by ActiveSupport still...

[seejohnrun]

@Tybot204 but isn't use_zone from ActiveSupport?

[Tybot204]

@seejohnrun Ya I edited my post above to say that. I didn't realize it was an extension on Time from ActiveSupport.

[seejohnrun]

Ah - sorry didn't see that @Tybot204

[seejohnrun]

Also - I guess I could be down to add ActiveSupport as a dependency, but
ideally we wouldn't

On Wed, Jul 6, 2016 at 1:25 PM Tyler Hogan [email protected] wrote:

@seejohnrun https://github.com/seejohnrun Ya I edited my post above to
say that. I didn't realize it was an extension on Time from ActiveSupport.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#335 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAD9xXAFrhDjbHH9zFtyUSt5cWIv918uks5qS-UFgaJpZM4IdduF
.

[dcosson]

Yeah, so I think the options are to include ActiveSupport as a dependency or raise an exception when parsing an ical string if it has a TZID but ActiveSupport is not available.

I guess I can see pros & cons to both - it's obviously an extra dependency, but by not requiring it as a gem dependency people might think they have this library fully installed and then unexpectedly get exceptions in production the first time they try to parse strings with tzid.

[seejohnrun]

How about when they want the parser having them require in that separately
and requiring ActiveSupport from there instead of in dependencies?

Another idea would be a separate gem but that feels like overkill
On Wed, Jul 6, 2016 at 4:23 PM Danny Cosson [email protected]
wrote:

Yeah, so I think the options are to include ActiveSupport as a dependency
or raise an exception when parsing an ical string if it has a TZID but
ActiveSupport is not available.

I guess I can see pros & cons to both - it's obviously an extra
dependency, but by not requiring it people might think they are good to go
and then unexpectedly get exceptions in production the first time they try
to parse strings with tzid.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#335 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAD9xbe3X3asW6azAiUb6ETXWy-3BF75ks5qTA6ngaJpZM4IdduF
.

[dcosson]

@seejohnrun not quite sure I understand the first suggestion. Unless I'm forgetting something, you don't interact with the parser directly, you call e.g. IceCube::Schedule.from_ical(ical_string) to parse the ical string. It seems inconvenient to have a hidden dependency (which is not a gem-level dependency) on ActiveRecord for anyone using the from_* methods, because it seems like the majority of people using this library will be serializing & deserializing at some point and so will need to have ActiveRecord.

[gma]

I'm strongly in favour of making dependencies on libraries like ActiveSupport optional, preferably ignoring them altogether. And let's not forget that people who serialize data don't automatically choose ActiveRecord – there are good reasons to choose different ways of storing data, or different libraries for interacting with a relational DB.

@dcosson What I think @seejohnrun meant when he said "How about when they want the parser having them require in that separately" was that people who need the functionality that depends on ActiveSupport could require that gem themselves (e.g. add it to their bundle) in order to disable the exception you proposed, and enable the functionality.

[seejohnrun]

Agree with @gma

I also think we could make it even nicer if we provide a wrapper around the active_support require, maybe something like:

require 'ice_cube/parsing' # which requires active_support

Parser.schedule_from_yaml(yaml)

just an idea - but definitely don't want to add AS as a dependency

[dimerman]

@seejohnrun @dcosson
this PR is about to be a year old. Any idea when/if it will be accepted ?

thanks

[seejohnrun]

@dimerman I'll try to get this worked up as suggested above this week some time. I definitely want to get this in, just don't want to introduce an AS requirement for those who don't want to add it.