docs: fix references in public docs - #2937
Conversation
dannycjones
left a comment
dannycjones
left a comment
There was a problem hiding this comment.
Thanks for this change, @JosephLenton! I think it's important that we maintain good hygiene on the docs.
I think there's a few things highlighted by the lints where we're better addressing the root cause. For example: types that we thought were public but actually aren't, and need exporting.
There was a problem hiding this comment.
I think there's something wrong with JoinHandle here. Maybe we should be exporting it rather than removing the references?
What do you think, @CTTY?
| /// Returns [`highest_field_id`]. | ||
| /// Returns the `highest_field_id`. | ||
| #[inline] | ||
| pub fn highest_field_id(&self) -> i32 { | ||
| self.highest_field_id | ||
| } |
There was a problem hiding this comment.
nitpick: I'd rather we describe what this method is returning, rather than referring to internal struct details. It's the reason really why the lint is failing in the first place. In the future, we might not even store this field and instead compute it on-demand.
i.e.
/// Returns the highest field ID assigned in this schema.
#[inline]
pub fn highest_field_id(&self) -> i32 {
self.highest_field_id
}The reason for this field is so we can quickly discover the highest field ID, and then start allocating new ones during schema evolution.
|
|
||
| impl SnapshotLog { | ||
| /// Returns the last updated timestamp as a DateTime<Utc> with millisecond precision | ||
| /// Returns the last updated timestamp as a `DateTime<Utc>` with millisecond precision |
There was a problem hiding this comment.
This is one we can and should link to!
| /// 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`. |
There was a problem hiding this 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 and usable (such as storing it in their own struct, should they wish to do so).
There was a problem hiding this comment.
I can see on the docs there are many types made, which aren't publicly shown: https://docs.rs/iceberg/latest/iceberg/transaction/struct.Transaction.html
My own opinion is everything named a public API should also be public. I'll make FastAppendAction, ExpireSnapshotsAction, ReplaceSortOrderAction, UpdateLocationAction, UpdatePropertiesAction, UpdateSchemaAction, UpdateStatisticsAction, and UpgradeFormatVersionAction, all public as well (these are all public items referenced from the Transaction object).
Their constructors are a mix of public and crate constructors. For now @dannycjones should I make them all pub(crate)? This would be the smallest change to the API (given they weren't public before).
2 participants
Which issue does this PR close?
What changes are included in this PR?
https://example.comto<https://example.com>.Are these changes tested?
These changes were QA'd by running
cargo doc --no-deps.AI Disclosure
I got Claude to do most of the changes in this PR.