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

No builder for TableMetadata and no public field #52

Closed
y0psolo opened this issue Aug 31, 2023 · 4 comments
Closed

No builder for TableMetadata and no public field #52

y0psolo opened this issue Aug 31, 2023 · 4 comments

Comments

@y0psolo
Copy link
Contributor

y0psolo commented Aug 31, 2023

There is no way to create a new TableMetadata struct outside a deserialization operation. I don't know if it's intended but all other struct have either builder or public field.

By the way i dont see clearly a consistent way of building API struct. Sometimes there is a builder and no public field, sometimes public field and no builder and sometimes builder and public field.

To have a better visibility on this I could write, if you want, a short table summary if needed with all API struct and two column : pub field (yes/no), builder (yes/no)

@liurenjie1024
Copy link
Contributor

Hi, @y0psolo Thanks for the contribution. We had a discussion in #3 about the style of public api, in conclusion

  1. For complex structs such as TableMetadata, Sanpshot, Schema, etc, we prefer not to expose pub field, but only necessary methods
  2. I'm ok with builder pattern.

cc @Xuanwo @ZENOTME @JanKaul Any comments?

@JanKaul
Copy link
Collaborator

JanKaul commented Sep 8, 2023

I would also be in favor of using the builder pattern for the pub structs.

If I'm correct all pub structs except for TableMetadata already have a builder. With the derive_builder crate it should be quite easy to implement the buider for TableMetadata.

@y0psolo
Copy link
Contributor Author

y0psolo commented Sep 13, 2023

i will try to start working on it next week. Not too much spare time till now.

@Fokko
Copy link
Contributor

Fokko commented Nov 27, 2024

Now #587 is in, I'll go ahead and close this issue 👍

@Fokko Fokko closed this as completed Nov 27, 2024
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

4 participants