property parameter refactor

[ModProg]

feat: new way of storing/setting properties

All properties can now be set using Component::property and Component::multi_property to overwrite an existing value and Component::property_appended and Component::multi_property_appended to append to the existing value. This is achieved through implementing From<ToString> for Property With this change Component::with_property_set and Component::with_property_appended are only for internal use.

All properties are now stored in a single Map<String, Vec<Property>> (both multi properties and single value properties). The key is now only stored through the map and no longer duplicated in the Property therefor Component::with_property_set and Component::with_property_appended take the tuple (Key, Value) now. The same is true for parameters.

For consistency Calendar behaves the same way only that Calendar::property always appends, because Calendar still uses a Vec to store properties.

BREAKING CHANGE: setting properties and parameters changed for Calendar and Components

[ModProg]

This is based on #24. This is a more fundamental refactor, that really only makes sense in the picture of parsing and implementing getters for standard properties.

[hoodie]

Haven't read the code yet, but does this mean that you can't have duplicate keys?

[ModProg]

Haven't read the code yet, but does this mean that you can't have duplicate keys?

you can, either by calling Component::property_appended twice or using Component::multi_property parsing something like ["A", "B"].

[ModProg]

But if all of these changes make sense depends on your future plans so this probably cannot be pushed as is :smile:

[hoodie]

I'm reading through the changes and I have to admit I have a bunch of questions. Can you tell me a bit about your motivation here and why you'd like to make these refactorings. Because it seems a bit to me like we're not talking about the same things.

[hoodie]

Maybe I can start with where I'm coming from: I was going to merge that parser branch at some point and I was thinking to make the underlying data structures look more similar to that of the parser's output (more tree, less hashmap style). So a calendar would be a Vec<Component> and a component would have a Vec<Property> etc.

Now that is very different from what you are building here, but my data structure also has a bunch of downsides. So I'm currently thinking: a: does this library now need two separateCalendar types, one for parsing/serializing and one for the builder-pattern. or b: is all you need different methods for the builder pattern? because then the underlying datastructure is not all that important.

I have to admit also: I don't really like the method names that you picked in this PR, they seem less descriptive than what's already there. This is why I'm having a hard time understanding what you want from this.

[ModProg]

Maybe I was a little over ambitious with this, I see where your confusion comes from, I also had a hard time thinking about what to name what.

So these are the things that I think could be nice, and I tried building with this.

• I wanted to use Into<Property> to allow for a single method for setting both simple values by passing a string, and more complex values by passing the Property struct.
• I wanted a way to not only set and append but also replace Properties, as this would be required when a component is supposed to be edited. (Previously this was only possible for normal properties, but not multi properties)
• I wanted a singular Structure for both the normal and the multi properties, at least from the outside, allowing to append to a normal property.
• To allow read access to normal and multi properties, I wanted to have them in a Map<Key, Property>. This could also be realized by having a get_property method that just searches/filters Vec<Property. (This is not really implemented by this, but would be the next step for my approach.)

a: does this library now need two separateCalendar types, one for parsing/serializing and one for the builder-pattern.

I hope not, I would like to have a datatype, that would just be built by the parser, and then can be used with the builder + getter functions (which don't exist yet) and then can be serialized back to the String.

I have to admit also: I don't really like the method names that you picked in this PR, they seem less descriptive than what's already there. This is why I'm having a hard time understanding what you want from this.

That's alright, I am definitely not the best at naming stuff, and we can use better names.

TLDR

• Support a simple form of accessing and setting Properties
• Support a methods/fields for standard properties and types for summary, start, location etc.
• And all of this is in (at least to the user) a single Datatype.

Possible Public API

This could be implemented for all possible data Structures really:

• Map<Key, Vec<Property>>
• Map<Key, Property> + Vec<Property>
• Vec<Property>

Generic Properties

• clear_property() -> Self
• get_property(key:String) -> Option<Property> or reference
• get_properties(key:String) -> Vec<Property> or slice
• append_property(key:String, Into<Property>) -> Self
• replace_property(key:String, Into<Property>) -> Self (is really just clear + append)
• maybe plural variants for append/replace

Standard Properties

• start(DateTime) -> Self
• get_start() -> Option<DateTime>
• remove_start() -> Self

Standard Multi Properties

These should maybe also have an append.

[hoodie]

I just finished this pending review, sorry if some comments are a bit out of date, I wrote this before writing my comment

[ModProg]

I just finished this pending review, sorry if some comments are a bit out of date, I wrote this before writing my comment

That's alright, Sorry to just dump so much code on you. I should have better explained my ideas beforehand.

[ModProg]

Hopefully my comment https://github.com/hoodie/icalendar-rs/pull/25#issuecomment-954890582 especially the lower half explains what I want to achieve, and maybe you have a better idea for data handling and api naming than me :laughing:

[hoodie]

Sorry to just dump so much code on you. I should have better explained my ideas beforehand.

It's ok, I like if people chime in on stuff. I'll take it into consideration, but I think I'll give the parser work preferential treatment for now and then see how we can incorporate both things.

[ModProg]

It's ok, I like if people chime in on stuff. I'll take it into consideration, but I think I'll give the parser work preferential treatment for now and then see how we can incorporate both things.

That's fair, like I said, in the end the internal data structure is not that relevant for me and I would be fine with just implementing the methods outlined above on what ever data structure you settle on.