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

Added RFC7986 properties to calendar component #198

Closed
wants to merge 1 commit into from

Conversation

espen
Copy link

@espen espen commented Nov 2, 2018

No description provided.

Copy link
Collaborator

@rahearn rahearn left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @espen. In addition to the changes that I mention in-line, I'd really like us to support the recommendations in section 3 about outputting the VALUE attribute to maintain backwards-compatibility. A test or two demonstrating compliance would be very much appreciated as well.

@@ -5,6 +5,17 @@ class Calendar < Component
required_property :prodid
optional_single_property :calscale
optional_single_property :ip_method
optional_single_property :ip_name
Copy link
Collaborator

Choose a reason for hiding this comment

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

Several of these properties can actually be defined more than once, so they should use the optional_property method instead of optional_single_property On the calendar component, they are name, description, categories, and image

Suggested change
optional_single_property :ip_name
optional_property :ip_name

optional_single_property :source, Icalendar::Values::Uri
optional_single_property :color
optional_single_property :image, Icalendar::Values::Uri
optional_single_property :conference, Icalendar::Values::Uri
Copy link
Collaborator

Choose a reason for hiding this comment

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

The conference property was not added to the Calendar component, but should instead be added to the Event and Todo components (where they should use the optional_property method). Several of the other properties should also be added to additional components in addition to the Calendar.

@rahearn
Copy link
Collaborator

rahearn commented Nov 3, 2018

To be more clear, the tests I am interested in are around compliance with section 3. I don't think we need them for just adding the new properties.

Also, if you think that part is too large for you to be able to get to, or over your head, just let me know and I can take a look at either offering suggestions or take over implementing it.

@estani
Copy link

estani commented Jan 21, 2020

This wouldn't have caused any troubles, it's a pity it wasn't merged.
Perhaps a more general approach would be to allow for a hash whose keys will go thorough .to_s.upcase (afaict ical attributes must be uppercase always) and it's values gone through .to_s

much like:

cal.event.other_attributes = { color: '#FFFFFF'}
# ... when writing other attributes
other_attributes_str = other_attributes.map { |k, v| "#{k.to_s.upcase}:#{v}"}.join("\n")

same for parsing, everything that is unknown could end up in that hash.

This will make the library more robust to changes.
My 2c.

@espen espen mentioned this pull request Jan 31, 2020
phamhoaivu911 added a commit to camelohq/icalendar that referenced this pull request Jun 21, 2022
@rahearn rahearn mentioned this pull request Jul 10, 2022
@rahearn rahearn closed this in #271 Jul 10, 2022
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.

None yet

3 participants