-
Notifications
You must be signed in to change notification settings - Fork 1.9k
ci: add check for doc comment formatting #18542
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
base: main
Are you sure you want to change the base?
ci: add check for doc comment formatting #18542
Conversation
cadd5fb to
6f7c4fa
Compare
305c31c to
2c1d0d6
Compare
2c1d0d6 to
0ed928f
Compare
|
Hi @alamb , Following the discussion in PR #16916, I've prepared this PR to enable CI enforcement for doc comment formatting. As you mentioned, we need to use the same version of This PR contains two commits:
PTAL when you have chance. Thank you! |
alamb
left a comment
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.
Thanks @CuteChuanChuan -- this looks really close. I think we just need to update where the command is run and this will be good to go from my perspective
Thank you for sticking with it
- Install nightly Rust toolchain with rustfmt component - Use cargo +nightly fmt to check both regular code and doc comments - Replace stable fmt check with nightly to avoid formatting conflicts
481a43d to
bb6cfcf
Compare
bb6cfcf to
3d932e2
Compare
3d932e2 to
dfdd028
Compare
|
Hi @alamb , Sorry for the late response. I've moved the nightly fmt command to |
|
Hi @alamb , friendly ping on this when you have a moment. No rush! |
alamb
left a comment
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.
Thank you @CuteChuanChuan -- this make sense to me ❤️
alamb
left a comment
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 merged up from main and ran cargo fmt again. Thank you for this @CuteChuanChuan -- it is a nice improvement.
The only final thing I am worried about is the developer experience here -- specifically helping developers know how to fix the errors when they encounter them
Specifically, running the script only generates the error
$ ci/scripts/rust_fmt.sh
+ rustup toolchain install nightly --component rustfmt
info: syncing channel updates for 'nightly-aarch64-apple-darwin'
info: latest update on 2026-01-01, rust version 1.94.0-nightly (8d670b93d 2025-12-31)
info: component 'rustfmt' for target 'aarch64-apple-darwin' is up to date
nightly-aarch64-apple-darwin unchanged - rustc 1.94.0-nightly (8d670b93d 2025-12-31)
info: checking for self-update
+ cargo +nightly fmt --all -- --check --config format_code_in_doc_comments=true
Diff in /Users/andrewlamb/Software/datafusion2/datafusion/core/src/physical_planner.rs:1906:
/// splitting AND conjunctions into individual expressions.
/// Column qualifiers are stripped so expressions can be evaluated against
/// the TableProvider's schema.
-///
fn extract_dml_filters(input: &Arc<LogicalPlan>) -> Result<Vec<Expr>> {
let mut filters = Vec::new();
Diff in /Users/andrewlamb/Software/datafusion2/datafusion/core/src/physical_planner.rs:1944:
/// For UPDATE statements, the SQL planner encodes assignments as a projection
/// over the source table. This function extracts column name and expression pairs
/// from the projection. Column qualifiers are stripped from the expressions.
-///
fn extract_update_assignments(input: &Arc<LogicalPlan>) -> Result<Vec<(String, Expr)>> {
// The UPDATE input plan structure is:
// Projection(updated columns as expressions with aliases)I had to go digging into script to find out how to actually fix thins:
cargo +nightly fmt --all -- --config format_code_in_doc_comments=trueMaybe we can also add a comment (or an error message) that tells people how to fix this error if they encouter it
|
@alamb, |
Which issue does this PR close?
Rationale for this change
This PR adds CI enforcement to ensure all code examples in documentation comments are properly formatted to maintain consistent code formatting, including examples in doc comments.
What changes are included in this PR?
This PR adds a new CI check in the
check-fmtjob that:cargo +nightly fmt --all -- --check --config format_code_in_doc_comments=trueNote:
format_code_in_doc_commentsis currently an unstable feature.Are these changes tested?
The command is tested locally with:
Are there any user-facing changes?
No user-facing changes.