c-thiel commented on code in PR #1362: URL: https://github.com/apache/iceberg-rust/pull/1362#discussion_r2143118998
########## crates/iceberg/src/spec/table_metadata.rs: ########## @@ -101,7 +101,10 @@ pub const RESERVED_PROPERTIES: [&str; 9] = [ /// Reference to [`TableMetadata`]. pub type TableMetadataRef = Arc<TableMetadata>; -#[derive(Debug, PartialEq, Deserialize, Eq, Clone)] +#[derive(Debug, PartialEq, Deserialize, Eq, Clone, typed_builder::TypedBuilder)] +#[builder(builder_method(name=declarative_builder))] +#[builder(builder_type(name=TableMetadataDeclarativeBuilder, doc="Build a new [`TableMetadata`] in a declarative way. For imperative operations (e.g. `add_snapshot`) and creating new TableMetadata, use [`TableMetadataBuilder`] instead."))] Review Comment: > To be honest, I feel the `TypedBuilder` is a little hard to maintain in such a large struct, do you mind to create a standalone struct for this? I don't quite understand this suggestion - isn't the purpose of this PR to create `TableMetadata`? How would implementing it on another struct help? Regarding names, @liurenjie1024 would you be OK with the breaking change that Piotrs proposal implies? Renaming the existing `TableMetadataBuilder` to something else, then naming the new builder `TableMetadataBuilder` (which does something completely different and is used by far less people?) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
