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.

Sign up for GitHub

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

Jump to bottom

fix: support for multi-properties #82

Merged
merged 1 commit into from
Nov 22, 2023
Merged

fix: support for multi-properties #82

merged 1 commit into from
Nov 22, 2023

Conversation

daveterra
Copy link
Contributor

WIP: Add support for multi-properties in calendar parsing.

  • Updated InnerComponent multi_property from Vec to BTreeMap<String, Vec>
  • When converting into InnerComponent, split the properties by those that "May occur more than once" and all others
  • Added a basic test as proof of concept
  • Still to do is add filtering for other properties such as attach, cattegory, etc.

@daveterra
Copy link
Contributor Author

Here is the basic outline, let me know what needs to change or any other feedback. I'm still pretty new to Rust so I've very likely missed something, especially around lifetimes.

hoodie
hoodie reviewed Nov 15, 2023
View reviewed changes
src/components.rs
@@ -23,7 +23,7 @@ pub use venue::*;
#[derive(Debug, Default, PartialEq, Eq, Clone)]
pub(crate) struct InnerComponent {
pub properties: BTreeMap<String, Property>,
pub multi_properties: Vec<Property>,
pub multi_properties: BTreeMap<String, Vec<Property>>,
Copy link
Owner

Choose a reason for hiding this comment

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

Didn't you want to go with a multimap here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still can, it does add another dependencies and I thought this approach might be more simple. It also keeps multi_properties more similar to properties in type. Happy to change it to multimap if you prefer.

hoodie
hoodie reviewed Nov 15, 2023
View reviewed changes
src/parser/components.rs Outdated
properties: component
.properties
fn is_multi(property: &Property) -> bool {
property.name.to_string() == "ATTENDEE"
Copy link
Owner

Choose a reason for hiding this comment

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

seems it's only ever attendee properties right?

Copy link
Owner

Choose a reason for hiding this comment

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

this inner fn is a bit of a smell, I think this should be a proper thing of it's own with a test or at least some documentation and a reference to the RFC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I agree. I didn't like it so much myself but was debating on exactly where to put it. Also, yes, there are many more properties to be added but I had started with Attendee as a proof of concept. I will update this.

hoodie
hoodie reviewed Nov 15, 2023
View reviewed changes
src/components.rs Outdated
@@ -96,8 +107,10 @@ pub trait Component {
write_crlf!(out, "UID:{}", Uuid::new_v4())?;
}

for property in self.multi_properties() {
property.fmt_write(out)?;
for properties in self.multi_properties().values() {
Copy link
Owner

Choose a reason for hiding this comment

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

you might be able to use .flatten() here

hoodie
hoodie reviewed Nov 15, 2023
View reviewed changes
src/parser/components.rs Outdated
}

let (multi, single): (Vec<_>, Vec<_>) =
component.properties.into_iter().partition(is_multi);
Copy link
Owner

Choose a reason for hiding this comment

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

there is probably a way to create an Iterator over all the multi properties and one over all the non-multi properties and iterate over those without copying into two new Vecs

hoodie
hoodie reviewed Nov 15, 2023
View reviewed changes
src/parser/components.rs Outdated
@@ -271,6 +283,18 @@ pub fn component<'a, E: ParseError<&'a str> + ContextError<&'a str>>(
))
}

#[allow(dead_code)]
Copy link
Owner

Choose a reason for hiding this comment

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

dead code?

src/parser/components.rs
ATTENDEE;CUTYPE=INDIVIDUAL;EMAIL="[email protected]":mailto:[email protected]
ATTENDEE;CUTYPE=INDIVIDUAL;EMAIL="[email protected]":mailto:[email protected]
END:VEVENT
"#,
Copy link
Owner

Choose a reason for hiding this comment

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

for simplicity you could also just test parsing and reserializing this same event

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, interesting Idea. I will try this.

Copy link
Owner

Choose a reason for hiding this comment

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

nvm, don't have to, I tried, it's a bit of a waste of time ;)

hoodie
hoodie reviewed Nov 17, 2023
View reviewed changes
src/parser/properties.rs Outdated
// [RFC-5545](https://datatracker.ietf.org/doc/html/rfc5545) states that the following
// "MAY occur more than once" in a VEVENT, VTODO, VJOURNAL, and VFREEBUSY.
// Note: A VJOURNAL can also contain multiple DECRIPTION but this is not covered here.
[
Copy link
Owner

Choose a reason for hiding this comment

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

this could be const actually, maybe define somewhere at module level

hoodie
hoodie reviewed Nov 17, 2023
View reviewed changes
src/parser/properties.rs Outdated
@@ -59,6 +59,28 @@ impl Property<'_> {
write_crlf!(out, "{}", fold_line(&line))?;
Ok(())
}

pub(crate) fn is_multi_property(&self) -> bool {
// [RFC-5545](https://datatracker.ietf.org/doc/html/rfc5545) states that the following
Copy link
Owner

Choose a reason for hiding this comment

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

nice comment, please make this a /// comment at the outside of the function

@hoodie
Copy link
Owner

hoodie commented Nov 18, 2023

@daveterra I'll take this over if you don't mind

@daveterra
Copy link
Contributor Author

@daveterra I'll take this over if you don't mind

Sounds good! Let me know if there is anything else I can help with.

@daveterra @hoodie
fix: parsing multi-properties
c0ffee9 
the parser now differentiates between different kinds of properties that
may occur multiple times in a component and will no longer override them
with each other.
@hoodie hoodie force-pushed the multi-properties branch 2 times, most recently from e3ce329 to c0ffee9 Compare November 22, 2023 11:05
@hoodie hoodie changed the title feat: add support for multi-properties fix: support for multi-properties Nov 22, 2023
@hoodie hoodie merged commit dcdb0b1 into hoodie:main Nov 22, 2023
10 checks passed
@hoodie
Copy link
Owner

hoodie commented Nov 22, 2023

thank you so much for finding this and helping out with the fix!!!

@hoodie
Copy link
Owner

hoodie commented Nov 22, 2023

https://crates.io/crates/icalendar/0.15.9

@qwandor
Copy link
Contributor

qwandor commented Nov 22, 2023

This should be a semver-breaking change, as it changes the signature of the public Component::multi_properties method.

@hoodie
Copy link
Owner

hoodie commented Nov 22, 2023

Oh no you're right. I had run semver checks but it didn't report that. I'll have that version.

@hoodie
Copy link
Owner

hoodie commented Nov 22, 2023

@qwandor thanks for ringing the alarm, it's 16.0.0 now
👏

@qwandor
Copy link
Contributor

qwandor commented Nov 22, 2023

I assume you mean 0.16.0? Thanks!

@hoodie
Copy link
Owner

hoodie commented Nov 22, 2023

of course

@hoodie hoodie mentioned this pull request Nov 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hoodie hoodie hoodie left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@daveterra @hoodie @qwandor