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

Issues/50 from ical #226

Closed
wants to merge 52 commits into from
Closed

Conversation

spra85
Copy link

@spra85 spra85 commented Apr 28, 2014

This is new, freshly rebased PR for continuing from_ical issue implementation -

@spra85
Copy link
Author

spra85 commented Apr 29, 2014

Referencing old issue (#86)

@mooremo
Copy link

mooremo commented Apr 30, 2014

👍 nice work

@quasor
Copy link

quasor commented May 10, 2014

Ooh, can't wait 👍

@btucker btucker mentioned this pull request May 10, 2014
@@ -142,12 +147,25 @@ def self.wday_to_sym(wday)
end
end

# Convert a symbol to an ical day (SU, MO)
def self.week_start(sym)
raise "No such day: #{sym}" unless DAYS.keys.include?(sym)
Copy link

Choose a reason for hiding this comment

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

It might be nice to class these exceptions to they're easier to rescue.

Copy link
Author

Choose a reason for hiding this comment

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

Just pushed up a change, thanks for the feedback @btucker

Copy link

Choose a reason for hiding this comment

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

Awesome, thanks!!

@spra85
Copy link
Author

spra85 commented May 19, 2014

@avit & @seejohnrun ... anything else we need to do to get this merged in?

@marshallshen
Copy link

👍

@seejohnrun
Copy link
Collaborator

i'm down - what do you think @avit?

@avit
Copy link
Collaborator

avit commented May 24, 2014

Reviewing today, I'll let you know if there's anything obvious otherwise it's going in.

It's taken so long because we don't use this functionality, but I think it's a disservice to the project to leave it out any longer. Let's get it in there and if any issues come up, we'll try and support it with everyone's help and contributions.

@quasor
Copy link

quasor commented May 24, 2014

Thank you guys!

@avit avit added the feature label May 24, 2014
@mooremo
Copy link

mooremo commented May 28, 2014

@spra85, good work 👍. Excited to see this merged

@spra85
Copy link
Author

spra85 commented May 28, 2014

Thanks @mooremo ... but have to deflect any thanks to everybody else in the commit log for this PR. All I did is deal with the rebase and move some logic around. Others did the meat of the work.

@avit
Copy link
Collaborator

avit commented May 28, 2014

I looked at this on the weekend. There's a lot of stuff happening in the commit log that should not be a part of this change:

  • Changing gem dependencies (don't want to change "activesupport" from ">= 3.0" to ">= 4.0")
  • Committing old (0.7.*) .gem builds into the repo
  • etc.

I'm going to flatten it all down to a single commit to clean it out. It'll break the history in the commit log for this branch, but there's like 4 years of back-and-forth integration between various people's forks in there and it's a mess to follow the actual relevant changes.

I also looked into using the icalendar gem to deal with the parsing & output to ical. It might be a better approach in the long run, but for now we can merge the current implementation, pending cleanup.

@mooremo
Copy link

mooremo commented May 29, 2014

Well then 👍 to everyone else too 😄

uris77 and others added 3 commits July 11, 2014 13:33
Instead of raising an exception, we have decided to ignore BYSETPOS for
now.
@wvengen
Copy link
Contributor

wvengen commented Sep 30, 2014

--> #258

@flivni
Copy link

flivni commented Dec 2, 2014

I think there is an issue/bug where dates are parsed in a recurrence rule. Sometimes dates are expressed sans the time component, e.g. “UNTIL=20150920”, and in this case we should use the time zone of the calendar rather than UTC.

@wvengen
Copy link
Contributor

wvengen commented Dec 3, 2014

Thanks - I can see this might be an issue. Reading RFC2445 indeed UNTIL= accepts both DATE and DATETIME.

Looking into this in more detail, I'm getting the idea that this pull request doesn't actually supports timezones (e.g. tzid is parsed off, but not used in schedule_from_ical, and there is no check for a trailing Z and Date.parse doesn't seem to handle that either). Am I right?

Moving to icalendar can be useful, but that's probably a long stretch.

@seejohnrun
Copy link
Collaborator

merged roll-up commit by @wvengen now - so closing this down (please re-open if we're missing something)

huge thanks to everyone and sorry for the massive delay - I'm going to be more active on this project moving forward

@seejohnrun seejohnrun closed this May 27, 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.