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

Make CI error if a function has no documentation #12938

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Oct 15, 2024

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?

@alamb
Copy link
Contributor Author

alamb commented Oct 15, 2024

@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
@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
@@ -48,12 +49,13 @@ fn main() {
_ => {
panic!("Unknown function type: {}", function_type)
}
};
}?;

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

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

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.

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

Copy link
Contributor

@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
25 checks passed
@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
Labels
core Core DataFusion crate
Projects
None yet
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