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: Convert to rST, add CI, and fix warnings #1588

Merged
merged 10 commits into from
Mar 10, 2023
Merged

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented Oct 25, 2022

Description of proposed changes

See commit messages.

Testing

@victorlin victorlin self-assigned this Oct 25, 2022
@nextstrain-bot nextstrain-bot temporarily deployed to auspice-victorlin-fix-d-h3bk6y October 25, 2022 18:48 Inactive
@nextstrain-bot nextstrain-bot temporarily deployed to auspice-victorlin-fix-d-h3bk6y October 25, 2022 20:14 Inactive
@nextstrain-bot nextstrain-bot temporarily deployed to auspice-victorlin-fix-d-h3bk6y October 25, 2022 20:16 Inactive
jameshadfield
jameshadfield previously approved these changes Oct 25, 2022
Copy link
Member

@jameshadfield jameshadfield left a comment

Choose a reason for hiding this comment

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

Tested (most of) these in the RTD preview and they look good. Thanks!

@victorlin
Copy link
Member Author

There's a lot more in the output of make html:

customise-client/api.md:34: WARNING: None:any reference target not found: ./index.rst
customise-client/api.md:215: WARNING: None:any reference target not found: requests.html
customise-client/requests.md:25: WARNING: None:any reference target not found: ../server/api.html
customise-client/requests.md:25: WARNING: None:any reference target not found: api.html
introduction/how-to-run.md:68: WARNING: None:any reference target not found: ../customise-client/index
introduction/how-to-run.md:96: WARNING: None:any reference target not found: ../server/api.html
introduction/how-to-run.md:115: WARNING: None:any reference target not found: ../releases/v2.html
releases/v2.md:19: WARNING: None:any reference target not found: ../contributing/overview.html
releases/v2.md:159: WARNING: None:any reference target not found: ../advanced-functionality/second-trees.html
releases/v2.md:196: WARNING: None:any reference target not found: ../customise-client/requests.html
releases/v2.md:232: WARNING: None:any reference target not found: ../customise-client/introduction.html
server/api.md:: WARNING: Could not lex literal_block as "json". Highlighting skipped.

This doesn't include the link in Displaying multiple trees that's broken (it should be ../server/api not server/api). I'll mark this as draft and fix more broadly. Docs CI should be enabled here.

@victorlin victorlin marked this pull request as draft October 25, 2022 23:27
Follow-up to ea762db.
This uses a reusable workflow in the nextstrain/.github repo.
@victorlin victorlin force-pushed the victorlin/fix-docs branch from c1411ae to be8b9b7 Compare March 8, 2023 19:41
@victorlin victorlin changed the title Fix docs typos and broken links docs: add CI, fix existing warnings, add other improvements Mar 8, 2023
@victorlin victorlin force-pushed the victorlin/fix-docs branch 2 times, most recently from 9b1e832 to 5a02608 Compare March 8, 2023 23:49
This path can be used for files such as custom CSS, but absence of usage
results in a build warning which removal gets rid of.
These have been moved since nextstrain/nextstrain.org@8064627.

Use HEAD to future-proof for any changes to the default branch name.
The YAML frontmatter was used for an old way of hosting the docs¹.

When used with the current docs hosting setup², this syntax results in a
Sphinx build "error" (it is rendered with as "title: Changelog"):

    ERROR: Document or section may not begin with a transition.

Using a markdown heading removes the error and allows for intended
styling.

¹ https://nextstrain.github.io/auspice/
² https://docs.nextstrain.org/projects/auspice/en/stable/releases/changelog.html
@victorlin victorlin force-pushed the victorlin/fix-docs branch 2 times, most recently from 80610f6 to f0fda9d Compare March 9, 2023 18:12
@victorlin victorlin marked this pull request as ready for review March 9, 2023 18:13
Copy link
Member

@jameshadfield jameshadfield left a comment

Choose a reason for hiding this comment

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

Looks good - thanks! Only a couple of very minor changes. I read through the rendered docs pages, but I didn't click on every link to test them.

docs/server/api.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
docs/customise-client/requests.rst Outdated Show resolved Hide resolved
docs/introduction/how-to-run.rst Outdated Show resolved Hide resolved
Done using a script (below). Individual tweaks will come in subsequent
commits.

Skipped narratives files since those are not used in this project.

    files=(
        docs/advanced-functionality/drag-drop-csv-tsv.md
        docs/advanced-functionality/misc.md
        docs/advanced-functionality/second-trees.md
        docs/advanced-functionality/view-settings.md
        docs/customise-client/api.md
        docs/customise-client/requests.md
        docs/introduction/how-to-run.md
        docs/releases/v2.md
        docs/server/api.md
        docs/server/authentication.md
    )

    for file in "${files[@]}"; do
        pandoc -f markdown-smart -t rst-smart --wrap none "$file" > "${file%.md}.rst"
        rm "$file"
    done
@victorlin victorlin force-pushed the victorlin/fix-docs branch from f0fda9d to a178f04 Compare March 10, 2023 01:20
This fixes these limitations from the MD → rST auto-conversion:

1. It converted internal doc links to external links instead of using the
   :doc: command.

2. It did not properly link to implicit HTML ID "#anchor" refs. Do that
   with the :ref: command.

3. It failed to convert lists when there wasn't an empty line preceding the
   original MD list.

4. It did not take advantage of rST note/warning directives.
The link doesn't work properly since the symlink isn't included in the
rST tree.

Instead of including the symlink in the rST tree, remove it and
reference the file directly.
Previously, there was a docs build warning:

    WARNING: Could not lex literal_block as "json". Highlighting skipped.

This fixes the warning.

Pin pygments to a minimum of 2.12.0 since that is the earliest version
to support this type of comment¹.

¹ https://pygments.org/docs/changelog/#version-2-12-0
This should have been a part of 8e07c26,
but better late than never.

Co-authored-by: James Hadfield <[email protected]>
@corneliusroemer
Copy link
Member

corneliusroemer commented Mar 10, 2023

This looks like a massive PR! Is that all new stuff or did you copy it from somewhere? I like all the documentation improvements, just wondering whether they're new or copied.

Ah ok, it's conversion from markdown to rst. Would be good to have the commit names in the PR description - this saves clicks.

@victorlin victorlin changed the title docs: add CI, fix existing warnings, add other improvements docs: Convert to rST, add CI, and fix warnings Mar 10, 2023
@victorlin victorlin merged commit 57c27b3 into master Mar 10, 2023
@victorlin victorlin deleted the victorlin/fix-docs branch March 10, 2023 17:47
@victorlin
Copy link
Member Author

@corneliusroemer sorry, I forgot to update the title with one of the larger changes here (convert to rST). Hopefully it's more clear now.

Would be good to have the commit names in the PR description - this saves clicks.

I'm averse to this because commit names/hashes will change as the PR is refined, and it would be a hassle to update the PR description on every change. It only saves one click to the "commits" tab, which is much more browsable than the static links in the description.

@tsibley tsibley mentioned this pull request Mar 10, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

5 participants