-
Notifications
You must be signed in to change notification settings - Fork 770
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
Fix object_store docs and Add CI job #4684
Conversation
@@ -65,6 +65,20 @@ jobs: | |||
- name: Run clippy with all features and all targets | |||
run: cargo clippy --all-features --all-targets -- -D warnings | |||
|
|||
# test doc links still work | |||
docs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should also move the docs workflow into the arrow workflow, as it only applies to that workspace now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a docs workflow that checks the workspace: https://github.com/apache/arrow-rs/blob/master/.github/workflows/docs.yml
However, I think object_store
is not part of the main workspace which is why it is not covered:
Lines 20 to 42 in c618438
members = [ | |
"arrow", | |
"arrow-arith", | |
"arrow-array", | |
"arrow-buffer", | |
"arrow-cast", | |
"arrow-csv", | |
"arrow-data", | |
"arrow-flight", | |
"arrow-flight/gen", | |
"arrow-integration-test", | |
"arrow-integration-testing", | |
"arrow-ipc", | |
"arrow-json", | |
"arrow-ord", | |
"arrow-row", | |
"arrow-schema", | |
"arrow-select", | |
"arrow-string", | |
"parquet", | |
"parquet_derive", | |
"parquet_derive_test", | |
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some comments to try and clarify the need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I agree on the need, I was perhaps suggesting we remove the docs.yml and merge it into arrow.yml
@@ -17,7 +17,10 @@ | |||
|
|||
use crate::config::Parse; | |||
|
|||
/// Configure how to provide [`ObjectStore::copy_if_not_exists`] for [`AmazonS3`] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than trying to actually make the link to [AmazonS3
] work by conditionalizing the docs with the "grim" approach @tustvold describes in #4683 (comment) I opted to simply avoid the link, which I think is reasonable given this is in the aws
module and has S3
in the name
object_store/src/aws/copy.rs
Outdated
@@ -17,7 +17,10 @@ | |||
|
|||
use crate::config::Parse; | |||
|
|||
/// Configure how to provide [`ObjectStore::copy_if_not_exists`] for [`AmazonS3`] | |||
/// Configure how to provide [`ObjectStore::copy_if_not_exists`] for | |||
/// `AmazonS3`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// `AmazonS3`. | |
/// [`AmazonS3`](super::AmazonS3). |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call -- in 65f94a3
Which issue does this PR close?
Closes #4683
Rationale for this change
Docs are important and we don't want to find out they are broken after releasing a crate to crates.io
What changes are included in this PR?
Are there any user-facing changes?
No
Example CI failure: https://github.com/apache/arrow-rs/actions/runs/5832061836/job/15816723694?pr=4684
Example CI pass: https://github.com/apache/arrow-rs/actions/runs/5832147821/job/15816978755?pr=4684