Add timezone parsing / serializing #455
Conversation
| else | ||
| time_zone = time.respond_to?(:time_zone) ? time.time_zone.name : time.zone | ||
| ";TZID=#{time_zone}:#{IceCube::I18n.l(time, format: '%Y%m%dT%H%M%S')}" # local time specified | ||
| ":#{IceCube::I18n.l(time, format: '%Y%m%dT%H%M%S')}" # local time specified |
There was a problem hiding this comment.
As shown in the spec, dates in local / floating time should be serialized in the form :YYYYMMDDTHHMMSS. Previously they were serialized with ;TZID=${time.zone}:YYYYMMDDTHHMMSS.
While I don't think TZID=${time.zone} is invalid (if only because, as the spec states, there is no specified format for timezones), it isn't a local time and it also doesn't produce time zones which ActiveSupport can understand (ActiveSupport doesn't understand time zone abbreviations, which makes TZID=${time.zone} serializable but not parsable).
| when 'EXDATE' | ||
| data[:extimes] ||= [] | ||
| data[:extimes] += value.split(',').map { |v| TimeUtil.deserialize_time(v) } | ||
| data[:extimes] += value.split(',').map { |v| TimeUtil.deserialize_time({time: v, zone: time_zone}) } |
There was a problem hiding this comment.
I updated the TimeUtil.deserialize_time to handle a hash argument where the :zone key might equal nil.
Also, while the TimeUtil.deserialize_time method could already handle parsing times with zone information, the ical_parser never sent zone information to the deserialize_time() method.
| @@ -109,10 +109,10 @@ def self.deserialize_time(time_or_hash) | |||
| when Time, Date | |||
| time_or_hash | |||
| when DateTime | |||
| Time.local(time.year, time.month, time.day, time.hour, time.min, time.sec) | |||
| Time.local(time_or_hash.year, time_or_hash.month, time_or_hash.day, time_or_hash.hour, time_or_hash.min, time_or_hash.sec) | |||
There was a problem hiding this comment.
So I think that this was a bug. Not sure when, if ever, the when DateTime condition is triggered, but before you had Time.local(time.year ... but the variable time doesn't appear to be defined anywhere.
I think this was a copy-paste error from the serialize_time() method above.
| @@ -201,7 +201,7 @@ | |||
| it 'should default to to_ical using local time' do | |||
| time = Time.now | |||
| schedule = IceCube::Schedule.new(Time.now) | |||
| expect(schedule.to_ical).to eq("DTSTART;TZID=#{time.zone}:#{time.strftime('%Y%m%dT%H%M%S')}") # default false | |||
| expect(schedule.to_ical).to eq("DTSTART:#{time.strftime('%Y%m%dT%H%M%S')}") # default false | |||
There was a problem hiding this comment.
I would argue that this test's expectation was just plain incorrect before. As stated, it is testing to ensure a date is serialized in local time (i.e. without a TZID), but before the test expected a string with TZID.
| expect(schedule.to_ical).to eq("DTSTART;TZID=#{time.zone}:#{time.strftime('%Y%m%dT%H%M%S')}") # default false | ||
| expect(schedule.to_ical(false)).to eq("DTSTART;TZID=#{time.zone}:#{time.strftime('%Y%m%dT%H%M%S')}") | ||
| expect(schedule.to_ical).to eq("DTSTART:#{time.strftime('%Y%m%dT%H%M%S')}") # default false | ||
| expect(schedule.to_ical(false)).to eq("DTSTART:#{time.strftime('%Y%m%dT%H%M%S')}") |
There was a problem hiding this comment.
Similar to the above, this spec was updated to expect local times to be serialized as local times.
| @@ -5,17 +5,19 @@ def self.schedule_from_ical(ical_string, options = {}) | |||
| ical_string.each_line do |line| | |||
| (property, value) = line.split(':') | |||
| (property, tzid) = property.split(';') | |||
| (_, time_zone) = tzid.split('=') if tzid | |||
There was a problem hiding this comment.
I believe this won't always work - looking at the spec, a component may have many parameters, of the form <KEY>=<VALUE>, separated by semicolons. If the key is TZID, then the value is the time zone that you want. But it there are other keys too.
For example, this is a valid ical line: DTSTART;VALUE=DATE:20200525. The code here would parse this and get the time zone DATE, which is not correct. The VALUE parameter tells us that the value (after the colon - 20200525 should be parsed as a date.
Another example would be DTSTART;VALUE=DATE-TIME;TZID:US-Eastern:19980119T020000. Here we do have a time zone, but it would be discarded by the code we have, and instead the time zone would be DATE-TIME.
There was a problem hiding this comment.
@iainbeeston you're correct.
This might fix it:
(property, *params) = property.split(';')
params.map! { |p| p.split('=') }
time_zone = params.select { |p| p&.first&.upcase == "TZID" }.first&.last
value_type = params.select { |p| p&.first&.upcase == "VALUE" }.first&.lastTimeUtil.deserialize_time should also be updated so that the Hash case accepts a type key and handles both "DATE" and "DATE-TIME" cases (I don't think the "TIME" value is applicable to this library).
There was a problem hiding this comment.
I assume this still needs to be resolved?
|
|
||
| # Vagrant files | ||
| Vagrantfile | ||
| .vagrant | ||
| ubuntu* |
There was a problem hiding this comment.
I don't think we need these?
|
Hey @pacso, it's quite possible that the comment I left on Jan 14th, 2019 was the last time I seriously looked at Ruby code. I'm definitely rusty at this point and not in a position to work on this merge request. Feel free to either close this or pick up where I left off (feel free to make use of the code here however you want). I switched to typescript and now maintain rSchedule. |
|
Hi, Edit: I see that #526 should take care of it and is about to be merged. Fingers crossed :) |
This PR updates the
full_tzbranch with correct timezone parsing / serializing. I'll add some comments in the code to explain some of the changes. It uses theresponds_to?(:time_zone)method you already had in place to check forActiveSupport.full_tzbranch might (?) have full time zone support (Supports parsing, serializing, and iterating time with zones--I think that's everything).full_tzbranch only supports iterating time with zone's, which I believe the master branch already supports (?).