Skip to content

Full Timezone support when parsing ical#408

Open
seejohnrun wants to merge 16 commits into
masterfrom
full_tz
Open

Full Timezone support when parsing ical#408
seejohnrun wants to merge 16 commits into
masterfrom
full_tz

Conversation

@seejohnrun

@seejohnrun seejohnrun commented Aug 4, 2017

Copy link
Copy Markdown
Collaborator

This PR builds on the great work from #335 (thanks @dcosson)

I'll be removing the need for ActiveSupport here.

Also, added support for parsing RDATE while I'm in here.

Closes #335

@seejohnrun

Copy link
Copy Markdown
Collaborator Author

@avit do you think we should move ical parsing out of ice_cube proper as part of this? Then we could avoid having to wrap with active_support conditionals.

Comment thread ice_cube.gemspec Outdated
Comment thread lib/ice_cube/parsers/ical_parser.rb Outdated
def parse_in_tzid(value, tzid)
time_parser = Time
if tzid
time_parser = ActiveSupport::TimeZone.new(tzid.split('=')[1]) || Time

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Use rescue Time here if ActiveSupport is not loaded? Maybe something like IcalParser.time_parser can be determined statically for performance.

@avit

avit commented Aug 4, 2017

Copy link
Copy Markdown
Collaborator

I like the idea... are you thinking of a separate gem, or maybe just an optional require?

@seejohnrun

Copy link
Copy Markdown
Collaborator Author

I think the optional require makes sense, but separate gem also sounds like a decent idea to me. I just dislike that we're potentially having two different behaviors in the ical parser dependending if AS is loaded or not

@avit

avit commented Aug 14, 2017

Copy link
Copy Markdown
Collaborator

We have a dependency on ActiveSupport here. Maybe this can work with TimeUtil.deserialize_time by passing a hash to it.

@seejohnrun

Copy link
Copy Markdown
Collaborator Author

@avit Ah I like that idea - will update this soon

@dcosson

dcosson commented Jun 20, 2018

Copy link
Copy Markdown

Any update on this?

It's really a bummer that this has been roadblocked for years at this point, even though the current behavior which is completely broken w.r.t. daylight savings time, just to avoid the dependency on ActiveSupport. With all due respect, what's more important - having a library that works and solves people's problems, or keeping the dependencies down?

@seejohnrun

Copy link
Copy Markdown
Collaborator Author

What do people think of the compromise of just making this method raise if ActiveSupport hasn't been required and we're attempting to parse a time with zone? I'm not 100% against the idea of the added dependency anymore, but want to make sure we're doing it for a reason that is required. I'm sorry this has held you up @dcosson and hopefully this branch has been useful to you.

@dcosson

dcosson commented Jun 20, 2018

Copy link
Copy Markdown

No worries, thanks for your continuing work maintaining this library. Hopefully it's close now!

+1 from me on raising if ActiveSupport isn't available.

@jorroll

jorroll commented Jun 20, 2018

Copy link
Copy Markdown
Contributor

Seems like a good idea.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants