-
-
Notifications
You must be signed in to change notification settings - Fork 58
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
Serialize and deserialize ORGANIZER with params #47
base: main
Are you sure you want to change the base?
Conversation
2e0f883
to
902df35
Compare
@@ -74,6 +74,7 @@ defmodule ICalendar.Util.DeserializeTest do | |||
assert %Event{} = event | |||
end | |||
|
|||
@tag :skip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this skipped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to show that it is a breaking change. It no longer deserializes to a string. It deserializes to a map. This is the original test, I added a new test just below this one for the new functionality.
assert event.organizer == "mailto:[email protected]"
Will fail, now.
I left this in (but skipped so we have passing tests) so you can compare the old functionality with the new.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's update this test to pass now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's update this test to pass now
I duplicated in the test below with the new functionality.
icalendar/test/icalendar/util/deserialize_test.exs
Lines 107 to 134 in 902df35
test "Include ORGANIZER w/ params and ATTENDEEs in event" do | |
event = | |
""" | |
BEGIN:VEVENT | |
DTSTART:20190711T130000Z | |
DTEND:20190711T150000Z | |
DTSTAMP:20190719T195201Z | |
ORGANIZER;[email protected]:mailto:[email protected] | |
UID:7E212264-C604-4071-892B-E0A28048F1BA | |
ATTENDEE;CUTYPE=INDIVIDUAL;ROLE=REQ-PARTICIPANT;PARTSTAT=ACCEPTED;[email protected];X-NUM-GUESTS=0:mailto:[email protected] | |
ATTENDEE;CUTYPE=INDIVIDUAL;ROLE=REQ-PARTICIPANT;PARTSTAT=ACCEPTED;[email protected];X-NUM-GUESTS=0:mailto:[email protected] | |
ATTENDEE;CUTYPE=INDIVIDUAL;ROLE=REQ-PARTICIPANT;PARTSTAT=ACCEPTED;CN=James SM;X-NUM-GUESTS=0:mailto:[email protected] | |
CREATED:20190709T192336Z | |
DESCRIPTION: | |
LAST-MODIFIED:20190711T130843Z | |
LOCATION:In-person at Volta and https://zoom.us/j/12345678 | |
SEQUENCE:0 | |
STATUS:CONFIRMED | |
SUMMARY:Design session | |
END:VEVENT | |
""" | |
|> String.trim() | |
|> String.split("\n") | |
|> Deserialize.build_event() | |
assert %Event{organizer: organizer} = event | |
assert %{:original_value => "mailto:[email protected]", "CN" => "[email protected]"} = organizer | |
end |
So we can just delete this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lpil removed
Hi @lpil and @ryanwinchester 👋 I see this PR has stagnated, and I'm realising it's something I need. I think the delta is pretty small for a feature change like this, so I think with a small tweak we can maintain backwards compatibility and push this through.
The small tweak I'd suggest is simply using Elixir style function pattern matching
So just copying the vanilla Does all that make sense? Or have I misjudged? I'm happy to make these changes and update the PR. At the second I'm lacking the time, but I could contribute to maintenance, if you're wanting to share some of the responsibility @lpil In the meantime I'll make these changes on my own fork. Let me know. |
@jarrodmoldrich if you add this test back in, with your changes, does it pass? 79b9159#diff-38283140e12894618fc3e09cccbe6d5aa4bfc35942eb9da277360ede3077e77dL78-L105 |
I just ran |
Closes #46
This serializes and desializes
ORGANIZER
similar to howATTENDEE
works.The only problem is that this is a breaking change for deserializing since it's now a
map
(like attendee) and not astring
.One way we could make deserializing not a breaking change would be to pass in some "optional options"
opts \\ []
, but they'd have to be passed all the way through from:ICalendar.Deserialize.from_ics/1
->
ICalendar.Util.Deserialize.build_event/1
->
ICalendar.Util.Deserialize.parse_attrs/2
What do you think we should do?