Skip to content

Comments

Make CI error if a function has no documentation#12938

Merged
alamb merged 1 commit intoapache:mainfrom
alamb:alamb/undocumented_functions
Oct 25, 2024
Merged

Make CI error if a function has no documentation#12938
alamb merged 1 commit intoapache:mainfrom
alamb:alamb/undocumented_functions

Conversation

@alamb
Copy link
Contributor

@alamb alamb commented Oct 15, 2024 •
edited
Loading

Draft until we have completely migrated all function documentation. The CI will fail until all currently existing functions are documented

Which issue does this PR close?

Closes #12872

Rationale for this change

The idea here is to ensure that all functions are documented, and ensure this is the case via a CI test.

What changes are included in this PR?

Changes:
make the ./dev/update_function_docs.sh script error if there are undocumented functions

Here is an example failure:

(venv) andrewlamb@Andrews-MacBook-Pro-2:~/Software/datafusion$ ./dev/update_function_docs.sh
/Users/andrewlamb/Software/datafusion
Inserting header
Running CLI and inserting aggregate function docs table
   Compiling datafusion v42.0.0 (/Users/andrewlamb/Software/datafusion/datafusion/core)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.77s
     Running `target/debug/print_functions_docs aggregate`
INFO: The following functions do not have documentation:
  - regr_count
  - regr_avgx
  - regr_syy
  - regr_intercept
  - regr_slope
  - regr_sxy
  - regr_r2
  - regr_sxx
  - regr_avgy
Error: NotImplemented("Some functions do not have documentation. Please implement `documentation` for: {\"regr_count\", \"regr_avgx\", \"regr_syy\", \"regr_intercept\", \"regr_slope\", \"regr_sxy\", \"regr_r2\", \"regr_sxx\", \"regr_avgy\"}")
(venv) andrewlamb@Andrews-MacBook-Pro-2:~/Software/datafusion$

Are these changes tested?

Yes, I tested it manually and here is an example of it failing

Are there any user-facing changes?

@github-actions github-actions bot added the core Core DataFusion crate label Oct 15, 2024
@alamb alamb mentioned this pull request Oct 15, 2024
Closed
@alamb
Copy link
Contributor Author

alamb commented Oct 15, 2024

Example CI failure: https://github.com/apache/datafusion/actions/runs/11344725082/job/31550591277?pr=12938

@alamb alamb force-pushed the alamb/undocumented_functions branch from c8244ac to a8b4d38 Compare October 24, 2024 20:28
@alamb alamb marked this pull request as ready for review October 24, 2024 20:30
This was referenced Oct 24, 2024
Closed
Closed
@alamb alamb changed the title Make CI test error if a function is not documented Make CI error if a function has no documentation Oct 25, 2024
comphead
comphead reviewed Oct 25, 2024
View reviewed changes
datafusion/core/src/bin/print_functions_docs.rs
};
}?;

println!("{docs}");
Copy link
Contributor

@comphead comphead Oct 25, 2024

Choose a reason for hiding this comment

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

do we need this println?

Copy link
Contributor

@Omega359 Omega359 Oct 25, 2024

Choose a reason for hiding this comment

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

Yes. This is how the shell script that builds the .md files retrieves the text. see https://github.com/apache/datafusion/blob/main/dev/update_function_docs.sh for impl.

comphead
comphead approved these changes Oct 25, 2024
View reviewed changes
Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

lgtm thanks @alamb

timsaucer
timsaucer approved these changes Oct 25, 2024
View reviewed changes
Copy link
Member

@timsaucer timsaucer left a comment

Choose a reason for hiding this comment

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

Nice addition!

@alamb alamb merged commit 21cfd6c into apache:main Oct 25, 2024
@alamb
Copy link
Contributor Author

alamb commented Oct 25, 2024

Bam ! Thank you for the reviews @comphead @Omega359 and @timsaucer -- We are so close to getting the docs all migrated over

@alamb alamb deleted the alamb/undocumented_functions branch October 25, 2024 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@comphead comphead comphead approved these changes

@timsaucer timsaucer timsaucer approved these changes

+1 more reviewer

@Omega359 Omega359 Omega359 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

core Core DataFusion crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Make generate_function_docs.sh error if there is a function without documentation

4 participants

@alamb @Omega359 @comphead @timsaucer