-
Notifications
You must be signed in to change notification settings - Fork 1.5k
DOC: Execute docs examples in CI #3507
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?
DOC: Execute docs examples in CI #3507
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3507 +/- ##
=======================================
Coverage 97.10% 97.10%
=======================================
Files 57 57
Lines 9711 9711
Branches 1759 1759
=======================================
Hits 9430 9430
Misses 168 168
Partials 113 113 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @j-t-1 and @stefan6419846 May I ask you to pre-review overall approach? As for now implemented only execution via CI. So far we have 72 failures. See full list here If in general this approach is acceptable, then in scope of this PR I'll:
|
|
FYI Code Blocks in HTML rendered as before after I've replaced |
.github/workflows/github-ci.yaml
Outdated
| sphinx-build --nitpicky --fail-on-warning --keep-going --show-traceback --builder html docs build/sphinx/html | ||
| - name: Test docs examples | ||
| run: | | ||
| sphinx-build --nitpicky --fail-on-warning --keep-going --show-traceback --builder doctest docs build/sphinx/html |
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.
Are all of these parameters required for this call? I am especially not sure about the output directory, as just running doctests should not write any HTML in theory?
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.
These parameters do nothing. I've removed them. Thanks!
As for output directory - it is necessary. Extension sphinx.ext.doctest creates .doctrees sub-folder with *.doctree files. I suppose these files contain documentation in some universal format which than can be transformed to variety of different formats. Previous CI step (when we run sphinx-build ... --builder html ...) also creates this sub-folder. As I see Sphinx is smart enough and looks for outdated files. So it makes sense to point to build/sphinx/html folder.
|
Hi @stefan6419846 and @j-t-1 This PR is ready for review. Please check out updated description first. |
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 for the PR.
I have left some reviews comments where your changes were not clear to me. Please note that some comments might cover other files with similar patterns as well.
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.
@stefan6419846 Thanks a lot for the review! It is big and requires a lot of effort from your side. I appreciate it.
I've covered all your questions. Many of them were just fixed (and marked with 👍). As for the rest, I've added explanations and sometimes questions. Let me now what need to be done next in this PR
docs/user/add-watermark.md
Outdated
| page.merge_page(stamp, over=False) # here set to False for watermarking | ||
| writer.write("out.pdf") | ||
| writer.write("out-underlay.pdf") |
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.
Do we use "underlay" somewhere else as well? Otherwise I would go with consistent naming like "stamp" for over and "watermark" for under, which seems to be the current naming scheme?
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.
Done
docs/user/suppress-warnings.md
Outdated
| want the text. You might use pdfminer.six as a fallback and do this: | ||
|
|
||
| ```python | ||
| % We prefer not to execute doc examples for third-party package "pdfminer" used only in one code snippet |
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.
The package is named pdfminer.six:
| % We prefer not to execute doc examples for third-party package "pdfminer" used only in one code snippet | |
| % We prefer not to execute doc examples for third-party package "pdfminer.six" used in one code snippet only |
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.
Yeah... missed. It was mentioned 3 lines above 👍
| file = os.path.join(dst_root_dir, file_name) | ||
| if os.path.isfile(file): | ||
| if not has_files: | ||
| print("Docs page was not configured propery for running code examples") |
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 guess these should print to stderr instead of stdout?
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.
In case of stderr doctest ignores output see official doc. Just double-checked, message appears in output, but test not fail. My intension is opposite, force to use {testsetup} directive if doc examples spit output files.
docs/conf.py
Outdated
| print("Deleting unexpected file(s) in " + dst_root_dir) | ||
| has_files = True | ||
| print(f"- {{file_name}}") | ||
| os.remove(file) # We should not affect other tests |
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.
| os.remove(file) # We should not affect other tests | |
| os.remove(file) # Avoid side effects on other tests |
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.
Done
docs/conf.py
Outdated
| has_files = False | ||
| for file_name in os.listdir(dst_root_dir): | ||
| file = os.path.join(dst_root_dir, file_name) |
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.
Please avoid using the (previously) builtin name file to avoid confusion.
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.
Done... thanks. I've learned something new 👍
Closes #2610
In this PR added execution for all python code snippets in documentation via sphinx.ext.doctest
In this PR:
doctestbuild. Effectively we test:testcodedirective in*.mdfilesautoclassdirective in*.rstfilesdocs/_build/doctest/pypdf_testdirectoryhtmlanddoctest). Nowsphinx-buildtool is run indocsdirectory and we have output file structure indocs/_buildthat match:Read the Docsbuild. See example for this PRmaketool as described in Sphinx's official docs:skipif: Truesvgwriteandpdfminer.six. These can be added inrequirements/docs.in. Please let me know if we need them.P.S.
Read the Docshas nice feature to view diff betweenmainand PR branch. See diff for this PR