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 myst parser #874

Merged
merged 1 commit into from
Jan 23, 2024
Merged

Conversation

dgarcia360
Copy link
Contributor

@dgarcia360 dgarcia360 commented Dec 8, 2023

Related PR: scylladb/sphinx-scylladb-theme#803

This PR replaces Sphinx's deprecated extension recommonmark with myst-parser, as per https://sphinx-theme.scylladb.com/stable/configuration/markdown.html#recommonmark-parser.

How to test

Check the docs build and display without errors.

Comment on lines 174 to 176
[^1]: There is an optimisation implemented for LWT requests that routes them
to the replicas in the ring order (as it prevents contention due to Paxos conflicts),
so replicas in that case are not shuffled in groups at all.

[^**]: In order for the optimisation to be applied, LWT statements must be prepared before.
so replicas in that case are not shuffled in groups at all.
In order for the optimisation to be applied, LWT statements must be prepared before.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reason: MystParser raises an error when trying to include a footnote reference within a footnote.

@dgarcia360 dgarcia360 marked this pull request as ready for review December 26, 2023 10:22
Copy link
Contributor

@annastuchlik annastuchlik left a comment

Choose a reason for hiding this comment

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

LGTM

@annastuchlik
Copy link
Contributor

@piodul Could you review and merge this PR?

@piodul
Copy link
Collaborator

piodul commented Jan 9, 2024

I checked out the branch and generated the documentation with:

make -C docs clean && make -C docs multiversionpreview

Unfortunately, it looks like only top-level pages are generated, and their TOCs are not rendered properly:

image

I also get a ton of warnings like these:

/tmp/tmpjo_qlxb7/1a66cb61e3ccfe42646c8fc75dd310a28f93ea25/docs/source/connecting/authentication.md:4: WARNING: Non-consecutive header level increase; H1 to H6 [myst.header]
/tmp/tmpjo_qlxb7/1a66cb61e3ccfe42646c8fc75dd310a28f93ea25/docs/source/connecting/authentication.md:18: WARNING: Non-consecutive header level increase; H1 to H3 [myst.header]
/tmp/tmpjo_qlxb7/1a66cb61e3ccfe42646c8fc75dd310a28f93ea25/docs/source/connecting/tls.md:8: WARNING: Non-consecutive header level increase; H1 to H3 [myst.header]
/tmp/tmpjo_qlxb7/1a66cb61e3ccfe42646c8fc75dd310a28f93ea25/docs/source/connecting/tls.md:40: WARNING: Non-consecutive header level increase; H1 to H3 [myst.header]
/tmp/tmpjo_qlxb7/1a66cb61e3ccfe42646c8fc75dd310a28f93ea25/docs/source/data-types/primitive.md:3: WARNING: Non-consecutive header level increase; H1 to H3 [myst.header]
/tmp/tmpjo_qlxb7/1a66cb61e3ccfe42646c8fc75dd310a28f93ea25/docs/source/data-types/primitive.md:23: WARNING: Non-consecutive header level increase; H1 to H3 [myst.header]

Copy link
Collaborator

@piodul piodul left a comment

Choose a reason for hiding this comment

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

The documentation does not generate properly (with the method I tried, at least - but given that it's just a makefile command, it should work). Please fix it.

@piodul
Copy link
Collaborator

piodul commented Jan 9, 2024

Maybe the page I showed was not the best example because it contains links to subpages - and those pages are indeed generated unlike what I wrote earlier - but the toctree part shouldn't be there and it looks like menu on the left should be expanded to show the path to the current document.

This is the same page, but on the current https://rust-driver.docs.scylladb.com/:

image

Note the difference in the menu on the left.

@annastuchlik
Copy link
Contributor

@piodul Running multiversionpreview is more complex and we shouldn't do it to review individual PRs locally. See the details here.
Can you try make preview?

@piodul
Copy link
Collaborator

piodul commented Jan 9, 2024

@annastuchlik I tried with make preview and it works. However, I also tried again with multiversionpreview, this time using the procedure described in https://sphinx-theme.scylladb.com/stable/configuration/multiversion.html#previewing-production-builds:

  • If I try doing this and build the documentation on the current main, I don't get the issues I described here and the documentation looks ok.
  • If I add the fork as a remote, pull the PR's branch and then check it out (git remote add dgarcia360 https://github.com/dgarcia360/scylla-rust-driver/; git fetch dgarcia360; git checkout dgarcia/docs-myst-parser) and then try to build mutliversionpreview, I get the same issues again. I also tried merging the branch to main and then building, but I get the same issues again.

I'm a bit hesitant to merge this as I'm not sure whether the documentation will look alright after this PR is merged. There seems to be a difference in output between preview and multiversionpreview - which is not present as of now - so I'm not convinced after seeing the output of preview that it's ok to merge.

@annastuchlik
Copy link
Contributor

@dgarcia360 Could you have a look at the issues @piodul reported? The screenshots and errors are added in the comments above.

Copy link
Collaborator

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

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

I left a few comments. One other issue, not strictly related to this PR, but I just noticed it and you are touching this region:
When rendering docs using mdbook (mdbook build docs --open), toctree sections are not ignored, and it looks like this:
image

Those should be somehow marked so that mdbook ignores them.

docs/source/conf.py Outdated Show resolved Hide resolved
Comment on lines 4 to 5
###### Important: The default authentication credentials are sent in plain text to the server. For this reason, it is highly recommended that this be used in conjunction with client-to-node encryption (SSL), or in a trusted network environment.

## Important: The default authentication credentials are sent in plain text to the server. For this reason, it is highly recommended that this be used in conjunction with client-to-node encryption (SSL), or in a trusted network environment.
Copy link
Collaborator

@Lorak-mmk Lorak-mmk Jan 9, 2024

Choose a reason for hiding this comment

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

This was probably meant as a warning box, similar to the one near the beggining of values.md, so instead of reducing the number of # it should be changed accordingly.

Also, it looks like our docs don't render those boxes very well.
Our docs:
image

mdbook (mdbook build docs --open):
image

This should definitely be fixed, warnings are important and should be visible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the syntax to define admonitions with this new parser: https://myst-parser.readthedocs.io/en/latest/syntax/admonitions.html#admonition-types

And, the subset of admonitions supported by the theme: https://sphinx-theme.scylladb.com/stable/examples/admonitions.html

docs/source/load-balancing/default-policy.md Outdated Show resolved Hide resolved
docs/source/queries/paged.md Outdated Show resolved Hide resolved
docs/source/queries/result.md Outdated Show resolved Hide resolved
@annastuchlik
Copy link
Contributor

@Lorak-mmk It seems you're reviewing the docs on GitHub. When it comes to the docs, GitHub is just a fallback. The primary documentation is the published online docs - you can review them with the make preview command.
For this reason, I have to skip your feedback regarding rendering if it's connected with the GitHub look and feel.

@Lorak-mmk
Copy link
Collaborator

@Lorak-mmk It seems you're reviewing the docs on GitHub. When it comes to the docs, GitHub is just a fallback. The primary documentation is the published online docs - you can review them with the make preview command. For this reason, I have to skip your feedback regarding rendering if it's connected with the GitHub look and feel.

mdbook is a standard tool for generating Rust book docs, afaik it's not related to GitHub in any way. I strongly feel, that book docs in this repo should be compatible with both mdbook and official docs. Why?

  1. We use mdbook in CI, to run examples from the book to make sure they work correctly and prevent code rot.
  2. Most Rust developers are familiar with the format and the tool, and are used to reading generated content.
  3. It is also helpful for me as a developer, as it's much faster that make preview and doesn't require me to fight with the Python mess for an hour each time I use it, so I get quick feedback loop thanks to it.
  4. For me - and I know I'm not the only one - the look of official docs is much worse and harder to read. If I need to look something up, it's more pleasant to use mdbook output.

By the way, the only thing in my review that is related to mdbook is the toctree thing, everything else is about the official docs. If you still don't want to touch mdbook, feel free to ignore this specific comment, I'll try to fix the issue myself later.

@Lorak-mmk
Copy link
Collaborator

Lorak-mmk commented Jan 9, 2024

Oh, nevermind about the toctree issue @annastuchlik , I started to look into why it doesn't work and saw we already have a solution for that - there is docs/build_book.py script, which copies the source to new dir, removes eval_rst blocks and build the book using mdbook.
So I guess the only thing for this PR is to update this script, as it looks for

```eval_rst

and this PR changed the format to

```{eval_rst}

It should be a 1-line change.

@annastuchlik
Copy link
Contributor

I'll defer to the author @dgarcia360.

@dgarcia360
Copy link
Contributor Author

dgarcia360 commented Jan 16, 2024

I've rebased the branch and applied the suggested changes. To maintain simplicity in this pull request, I chose not to modify the H1-H6 headings order. Instead, I've suppressed the related warning through the conf.py file by using the suppress_warnings option.

@piodul

Unfortunately, it looks like only top-level pages are generated, and their TOCs are not rendered properly:

I believe v0.11.0 was not released at the time of creating the branch for this pull request. I've fixed the issue by adding it to the versions that should keep using recommonmark instead of Myst.

Copy link
Collaborator

@piodul piodul left a comment

Choose a reason for hiding this comment

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

I no longer see the issues I reported about make multiversionpreview, so looks good from my point of view.

@Lorak-mmk Lorak-mmk merged commit 2af0214 into scylladb:main Jan 23, 2024
9 checks passed
@Lorak-mmk Lorak-mmk mentioned this pull request Feb 1, 2024
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.

4 participants