search

hoodie / icalendar-rs

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

Create Builder/Query abstraction #35

Open hoodie opened 2 years ago

hoodie commented 2 years ago

Everything in this crate is currently only a builder, but now that we have a parser we cannot simply add getters to the calendar to read properties.

So we should add a Calendar::build() -> CalendarBuilder and move builder methods in there (same for Components respectively) and add a calendar.query() -> CalendarQuery with similar methods to that act as getters.

ThomasdenH commented 4 weeks ago

Could you expand more on what the idea would be?

I'm not yet very familiar with the library, but to me it would make sense to have objects like Alarm, Event, etc. that have regular Rust getters and setters, i.e. fn summary(&self) -> &str and fn set_summary(&mut self, summary: &str). You could then have builder (derive via typed_builder?). What would be the purpose of the Query structs?

ThomasdenH commented 3 weeks ago

Having toyed around with creating a custom strongly typed builder, I think it may be best to rewrite the components to actually have the properties as fields and use typed_builder to derive a builder. This has the advantage that it is easy to keep track of types and of which fields are set and which aren't. Serialization and deserialization would then be a little more work.

While possible to do this in a backwards compatible way, I think it would be good to rethink the API and make this a major version anyway. Let me know if you think this is a good idea. If you like I can make a draft of this.

hoodie commented 3 weeks ago

Thanks. This sounds like something I have been thinking about before, but shyed away from the work and churn this would cause. I'm not super happy with the way that we assamble components in the builder (Hashmaps for everything), but this was the easiest way at the time.

If you're up to it please be my guest and have a look, I'd appreciate the input. ATM I can conceive of two options:

  1. Special structs for every type of component and property
  2. Add only a .query() helper that returns a struct Query with getters
  3. At least combine the top level component types and the parser:: types (this duplication has been bothering me for a while)

Concerting [1]: I'm not sure if having specific structs for everything would be the way to go, as you can easily build something that is not extensible. At the very least we should put #[non_exhaustive] on each of them in order to be able to add properties later on. Concerning [3]: that would be a nice refactoring (lots of red -s in diff) but it would not be very visible.

As to the builder v. query api that this issue is actually about: I think an MVP would be to keep the current builder as is and mirror everything inside the Query type.