dannycjones commented on code in PR #2937:
URL: https://github.com/apache/iceberg-rust/pull/2937#discussion_r3704409558
##########
crates/iceberg/src/transaction/update_schema.rs:
##########
@@ -32,12 +32,11 @@ use crate::{Error, ErrorKind, Result, TableRequirement,
TableUpdate};
// Default ID for a new column. This will be re-assigned to a fresh ID at
commit time.
const DEFAULT_FIELD_ID: i32 = 0;
-/// Declarative specification for adding a column in [`UpdateSchemaAction`].
+/// Declarative specification for adding a column in an `UpdateSchemaAction`.
///
/// Use helper constructors such as [`AddColumn::optional`] and
[`AddColumn::required`],
-/// optionally combined with [`AddColumn::with_parent`] and
[`AddColumn::with_doc`], then pass
-/// the value to
-/// [`UpdateSchemaAction::add_column`].
+/// optionally combined with the builder's `parent` and `doc` setters via
+/// [`AddColumn::builder`], then pass the value to
`UpdateSchemaAction::add_column`.
Review Comment:
This is another place that I think we've got an issue with - we should
likely be exporting `UpdateSchemaAction` properly.
We shouldn't allow library consumers to construct `UpdateSchemaAction`
(that's already `pub(crate)`) other than via a transaction, but we should
export the type so that it's documented.
##########
crates/iceberg/src/transaction/update_schema.rs:
##########
@@ -32,12 +32,11 @@ use crate::{Error, ErrorKind, Result, TableRequirement,
TableUpdate};
// Default ID for a new column. This will be re-assigned to a fresh ID at
commit time.
const DEFAULT_FIELD_ID: i32 = 0;
-/// Declarative specification for adding a column in [`UpdateSchemaAction`].
+/// Declarative specification for adding a column in an `UpdateSchemaAction`.
///
/// Use helper constructors such as [`AddColumn::optional`] and
[`AddColumn::required`],
-/// optionally combined with [`AddColumn::with_parent`] and
[`AddColumn::with_doc`], then pass
-/// the value to
-/// [`UpdateSchemaAction::add_column`].
+/// optionally combined with the builder's `parent` and `doc` setters via
+/// [`AddColumn::builder`], then pass the value to
`UpdateSchemaAction::add_column`.
Review Comment:
This is another place that I think we've got an issue with - we should
likely be exporting `UpdateSchemaAction` properly.
We shouldn't allow library consumers to construct `UpdateSchemaAction`, but
we should export the type so that it's documented.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
