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

Docs: Add a book preprocessor #910

Merged
merged 2 commits into from
Jan 12, 2024
Merged

Conversation

Lorak-mmk
Copy link
Collaborator

This PR adds a preprocessor for the book that removes Sphinx-related sections.
This was previously done by docs/build_book.py script, which this PR removes.
There are some advantages of new approach:

  • No unnecessary file writing. Book content is passed to preprocessor by stdin. This is both faster and more friendly to disk.
  • No way to accidentally build a book without preprocessing. Also now developer doesn't need to know about the script or Makefile target that uses it.
  • New script is simpler as it doesn't need to handle files anymore. As a contributor I can certify that it's possible to not know about the script or Makefile target and be frustrated about Sphinx sections.

There is one disadvantage: book must be built either from main directory, or docs directory. If it's built from different location, build will succeed buy Sphinx sections won't be stripped.
This is because of mdbook limitation - PWD of build process is the folder the command is executed in, not the folder in which book.toml is located.
This shouldn't be a problem - I suspect book build is never / very rarely executed from other dir.
If it becomes a problem, it's possible to fix it. The script in bash.sh will need to be modified:

  • if executed with supports argument - just exit.
  • Otherwise, read stdin using jq. From first element extract root element, cd to it's value.
  • Execute python ./sphinx_preprocessor.py with previosuly-read stdin.

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@Lorak-mmk
Copy link
Collaborator Author

Submitted an issue to mdbook: rust-lang/mdBook#2289

@Lorak-mmk Lorak-mmk self-assigned this Jan 11, 2024
@Lorak-mmk
Copy link
Collaborator Author

mdbook issue was closed in favor of rust-lang/mdBook#1424 which is open and not touches for 3 years now, so we shouldn't hope for mdbook fix. Imo the script I wrote should be enough, if it isn't I can write more complicated one that would always work.

CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/sphinx_preprocessor.py Outdated Show resolved Hide resolved
@@ -0,0 +1,34 @@
import json
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: how about adding a shebang? We had it in the previous script.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This script is not supposed to be run manually, so I don't see how shebang helps?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense not to do it.

This is to avoid having to use build_book.py script.
1. Preprocessor is faster and doesn't use disk, as all communication is
   trough stdio.
2. It doesn't need to be executed explicitly. You can just use
   `mdbook build docs` and it will create correct documentation.
3. New script is shorter and simpler than old.

Script is based on example from https://rust-lang.github.io/mdBook/for_developers/preprocessors.html
and uses a function from build_book.py

In book.toml the command used is a simple bash script instead of just
`python ./sphinx_preprocessor.py` because the command is run in the
directory the user executed it in, not the directory that book.toml
is in (which is imho a poor design in this case), so in order to be able
to run the command both from main dir and from docs dir, such a script
locating the preprocessor is necessary.
This script is no longer necessary and just double the work
@Lorak-mmk
Copy link
Collaborator Author

v2

  • Changed contributing.md: added a note about limitations
  • Removed unnecessary in the script coment.

@Lorak-mmk Lorak-mmk requested a review from piodul January 12, 2024 12:02
@piodul piodul merged commit c9aecf4 into scylladb:main Jan 12, 2024
9 checks passed
@avelanarius avelanarius added this to the 0.12.0 milestone Jan 15, 2024
@Lorak-mmk Lorak-mmk mentioned this pull request Feb 1, 2024
@piodul piodul mentioned this pull request Feb 2, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants