hoodie / icalendar-rs

📆 icalendar library, in Rust of course - for fun
Apache License 2.0
122 stars 32 forks source link

Add getters for calendar and event properties #37

Closed qwandor closed 2 years ago

qwandor commented 2 years ago

Being able to parse an iCalendar file into a Calendar isn't much use if you can't then access the data in it in a useful way.

This PR adds typed getters to Calendar, Event, Todo, Venue and the Component trait, to get the same properties as the builder methods allow to be set.

I also:

hoodie commented 2 years ago

Thanks for the work, you're right, this is very much needed. I actually was thinking about this already. Currently the Component objects are only builders, because that is how the library started. I didn't want to riddle the scope with get_ functions though, that's why I started adding a query() method that would return a corresponding type with all the getters (without the get_ prefix. Unfortunately I got interrupted.

Have a look here here perhaps https://github.com/hoodie/icalendar-rs/commit/27231259fe898f3a4495d0f6bc3e7659c748d8d1

What do you think?

qwandor commented 2 years ago

That commit doesn't include the ComponentQuery type so it's hard to say, but it seems a bit back to front and more complicated than necessary. Personally I prefer not to add separate types for builders versus data objects in Rust, but if you do want to I think it would make more sense to have the done method on the builder return the data object, which is then used everywhere else (including being the result of parsing a file). Or indeed you could have From implementations to convert back and forth between builder and data object. This would be an API breaking change though.

My preference, which would also be a breaking change but more idiomatic I think, would be to keep the current set of types, but rename the setters to set_ and have no prefix for the getters.

qwandor commented 2 years ago

I've separated the traits part into #38 so we can get that in at least, but I'd still like to get some solution to the problem of accessing calendar data in soon as I want to use this in something I'm working on for parsing iCalendar files as well as generating them.

hoodie commented 2 years ago

actually this looks pretty good. Thanks for the contribution. I would like to preserve conventional commit messages here, so I would go ahead and just add the prefixes to your commits. unless you would like to do that yourself

qwandor commented 2 years ago

Done.

hoodie commented 2 years ago

Thanks for the changes, I just released this as 0.12.0

qwandor commented 2 years ago

Thanks!