Skip to content

Support RFC 5545 property names and custom properties#16

Merged
rickychilcott merged 6 commits intomasterfrom
meitar-issue-10
Apr 8, 2020
Merged

Support RFC 5545 property names and custom properties#16
rickychilcott merged 6 commits intomasterfrom
meitar-issue-10

Conversation

@rickychilcott
Copy link
Copy Markdown
Member

@rickychilcott rickychilcott commented Apr 8, 2020

Close #10
Close #14

@rickychilcott
Copy link
Copy Markdown
Member Author

@meitar can you give this a once over and see if this looks ok to you. I use iCalendar's properties (and custom priorities) to tell us what to expose as opposed to hard coding it.

It works for me, maybe you can test out in your context?

@fabacab
Copy link
Copy Markdown
Contributor

fabacab commented Apr 8, 2020

Hmm, using 8871be2 in my templates I get this error:

Liquid Exception: undefined method `dtstart' for #<Jekyll::IcalTag::Event:0x00007ff00c0f2b08> in /_layouts/iCalendar.ics

A --trace tells me the error is triggered on calendar_limiter.rb, line 17.

The relevant call to the plugin in my Jekyll layout is:

{% ical url: https://example.com/calendar.ics only_future: true limit: 5 %}

@rickychilcott
Copy link
Copy Markdown
Member Author

Whoops. You're right. Let me fix that up here right now.

@rickychilcott
Copy link
Copy Markdown
Member Author

fixed in 1834677. Try again @meitar ?

@fabacab
Copy link
Copy Markdown
Contributor

fabacab commented Apr 8, 2020

1834677 fixes the unexposed dtstart, but now I get the Liquid Exception: incompatible character encodings: ASCII-8BIT and UTF-8 error again. I think there the refactoring dropped the call to as_utf8 at some point.

Comment thread lib/jekyll-ical-tag.rb Outdated
Comment on lines +76 to +87
context["event"].transform_values do |value|
v = case value
when String
value.force_encoding("UTF-8")
when Date
value.to_time
else
value
end

v.presence
end
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Forcing encoding to utf8

Copy link
Copy Markdown
Contributor

@fabacab fabacab Apr 8, 2020

Choose a reason for hiding this comment

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

Line 78 in this file should also include a match against Icalendar::Values::Text, so I think the block should be:

Suggested change
context["event"].transform_values do |value|
v = case value
when String
value.force_encoding("UTF-8")
when Date
value.to_time
else
value
end
v.presence
end
context["event"].transform_values do |value|
v = case value
when String, Icalendar::Values::Text
value.force_encoding("UTF-8")
when Date
value.to_time
else
value
end
v.presence
end

See this comment.

@rickychilcott
Copy link
Copy Markdown
Member Author

rickychilcott commented Apr 8, 2020 via email

@fabacab
Copy link
Copy Markdown
Contributor

fabacab commented Apr 8, 2020

Do you have a feed that I can test against?

Here's one that fails with the above error for me: https://www.google.com/calendar/ical/ctftime%40gmail.com/public/basic.ics

@rickychilcott
Copy link
Copy Markdown
Member Author

Thanks I'll try with that. Also, which version of Ruby are you on?

@fabacab
Copy link
Copy Markdown
Contributor

fabacab commented Apr 8, 2020

Thanks I'll try with that. Also, which version of Ruby are you on?

$ ruby --version
ruby 2.6.5p114 (2019-10-01 revision 67812) [x86_64-darwin18]

@fabacab
Copy link
Copy Markdown
Contributor

fabacab commented Apr 8, 2020

I think it's failing because String is not the kind of object you're handling, it's actually Icalendar::Values::Text. So this works:

when String, Icalendar::Values::Text

but just String never calls the when block.

@rickychilcott
Copy link
Copy Markdown
Member Author

Yep. That was it. It should be fixed now. Sorry for that little dance.

@rickychilcott rickychilcott changed the title Support RFC 5545 property names and custom properities Support RFC 5545 property names and custom properties Apr 8, 2020
@fabacab
Copy link
Copy Markdown
Contributor

fabacab commented Apr 8, 2020

Yep. That was it. It should be fixed now.

Yup, this looks good to me. All my various templates build correctly now. If you publish this on RubyGems I can kill my fork. Thanks so much!

Sorry for that little dance.

No stress, and if I may say so, thanks for the quick replies. I'm not used to people responding so quickly, so I normally do a helluva lot more debugging on my own before I ever even think about responding. But you were, like, right there within seconds. Saved me a lot of time, and helped me hone my Ruby far less painfully than I'm used to in the process. Again, thanks. :)

@rickychilcott rickychilcott merged commit 2a9e29c into master Apr 8, 2020
@rickychilcott
Copy link
Copy Markdown
Member Author

Not a problem. That was fun over the past few days! Thanks for your great code contributions and for pushing this plugin to be better.

Best of luck and stay well in this season.

@rickychilcott rickychilcott deleted the meitar-issue-10 branch April 8, 2020 16:34
@rickychilcott
Copy link
Copy Markdown
Member Author

Version 1.1.0 has been published.

@fabacab
Copy link
Copy Markdown
Contributor

fabacab commented Apr 8, 2020

stay well in this season.

You too!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Proposal: Use RFC5545 attribute names for events

2 participants