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.

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

Create Builder/Query abstraction #35

Open
hoodie opened this issue Dec 19, 2021 · 3 comments
Open

Create Builder/Query abstraction #35

hoodie opened this issue Dec 19, 2021 · 3 comments
Milestone

Comments

@hoodie
Copy link
Owner

hoodie commented Dec 19, 2021

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.

@hoodie hoodie added this to the 0.11.0 milestone Dec 19, 2021
@ThomasdenH
Copy link
Contributor

ThomasdenH commented Oct 24, 2024

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
Copy link
Contributor

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
Copy link
Owner Author

hoodie commented Oct 28, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants