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

Fix documentation warnings, make CsvExecBuilder and Unparsed pub #11729

Merged
merged 4 commits into from
Aug 1, 2024

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Jul 30, 2024

Which issue does this PR close?

Closes #11728

Rationale for this change

Warnings in cargo doc sometimes mask real bugs as well as making it hard to track down cargo doc errors locally in my iDE

See also #11728

What changes are included in this PR?

  1. Update CI so broken links are treated as errors
  2. Fix all current warnings
  3. Make 2 structures pub so they can be used by users (caught by doc warnings)

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added sql SQL Planner development-process Related to development process of DataFusion optimizer Optimizer rules core Core DataFusion crate labels Jul 30, 2024
@@ -234,7 +234,7 @@ jobs:
rust-version: stable
- name: Run cargo doc
run: |
export RUSTDOCFLAGS="-D warnings -A rustdoc::private-intra-doc-links"
export RUSTDOCFLAGS="-D warnings"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes all doc warnings an error and does not allow links to private items

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RUSTDOCFLAGS in ci/scripts/rust_docs.sh should be updated as well
i am doing this in #11890

@@ -35,7 +35,7 @@ pub use self::parquet::{ParquetExec, ParquetFileMetrics, ParquetFileReaderFactor

pub use arrow_file::ArrowExec;
pub use avro::AvroExec;
pub use csv::{CsvConfig, CsvExec, CsvOpener};
pub use csv::{CsvConfig, CsvExec, CsvExecBuilder, CsvOpener};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this struct, added by @connec recently, actually wasn't publically exported which was flagged by one of the doc tests

@@ -191,9 +191,9 @@ pub use writer::plan_to_parquet;
/// # Execution Overview
///
/// * Step 1: [`ParquetExec::execute`] is called, returning a [`FileStream`]
/// configured to open parquet files with a [`ParquetOpener`].
/// configured to open parquet files with a `ParquetOpener`.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since ParquetOpener isn't public let's not try to make a link to it

@@ -29,6 +29,8 @@ pub use plan::plan_to_sql;
use self::dialect::{DefaultDialect, Dialect};
pub mod dialect;

pub use expr::Unparsed;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unparsed was also not pub, though maybe it does't need to be

Copy link
Member

@jonahgao jonahgao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me👍

datafusion/functions-aggregate/src/string_agg.rs Outdated Show resolved Hide resolved
@alamb
Copy link
Contributor Author

alamb commented Aug 1, 2024

Thanks @jonahgao for the review.

@alamb alamb merged commit a4ac082 into apache:main Aug 1, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate development-process Related to development process of DataFusion optimizer Optimizer rules sql SQL Planner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

doc warnings about private items
3 participants