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

Replace recommonmark with myst_parser and fix changelog rendering #575

Merged
merged 3 commits into from Mar 10, 2022

Conversation

rccern
Copy link
Contributor

@rccern rccern commented Feb 23, 2022

There are some issues with the docs hosted at https://jupyterhub-kubespawner.readthedocs.io/en/2.0.1/changelog.html , some of which are reported in #527 . I think they are in part due to issues with recommonmark, which has been discontinued in favor of myst-parser (readthedocs/recommonmark#221).
I also bumped the sphinx version to >=4.2 as versions below don't work with python 3.10 (see sphinx-doc/sphinx#9513 )

This PR aims to fix those issues.
PS. the page on RTD still points to 1.1.1 for latest, I don't know if that's intended


EDIT 1 by Erik: Closes #527, Closes #555
EDIT 2 by Erik: This doesn't resolve an upstream issue when our anchors are numeric though, but let's track that as part of executablebooks/MyST-Parser#527.

@welcome
Copy link

welcome bot commented Feb 23, 2022

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@consideRatio
Copy link
Member

WIEEEE thanks @rccern! This is work that have made me stumble in the past. This LGTM from code review and looking at the RTD PR preview.

@choldgraf with more experience about docs, does this LGTY also?

@consideRatio consideRatio changed the title fix markdown in docs TOC Fix broken anchors in docs by transitioning to MyST Feb 23, 2022
@choldgraf
Copy link
Member

choldgraf commented Feb 23, 2022

That generally looks good to me, though I think you might need to explicitly enable the anchor auto-generator, as described here: https://myst-parser.readthedocs.io/en/latest/syntax/optional.html#auto-generated-header-anchors

@rccern
Copy link
Contributor Author

rccern commented Feb 24, 2022

@choldgraf it seems i can't get it to work...
I added myst_heading_anchors = 2 to conf.py, which seems reflected properly in the sphinx-build output:

myst v0.17.0: MdParserConfig(commonmark_only=False, gfm_only=False, enable_extensions=[], linkify_fuzzy_links=True, dmath_allow_labels=True, dmath_allow_space=True, dmath_allow_digits=True, dmath_double_inline=False, update_mathjax=True, mathjax_classes='tex2jax_process|mathjax_process|math|output_area', disable_syntax=[], all_links_external=False, url_schemes=['http', 'https', 'mailto', 'ftp'], ref_domains=None, highlight_code_blocks=True, number_code_blocks=[], title_to_header=False, heading_anchors=2, heading_slug_func=None, html_meta=[], footnote_transition=True, substitutions=[], sub_delimiters=['{', '}'], words_per_minute=200)

if I run myst-anchors -l2 source/changelog.md I see the result I expect, i.e.

<h1 id="changes-in-kubespawner"></h1>
<h2 id="unreleased"></h2>
<h2 id="20"></h2>
<h2 id="11"></h2>
<h2 id="10"></h2>
<h2 id="016"></h2>
...

but the html shows

<li class="toctree-l1"><a class="reference internal" href="overview.html">Overview</a></li>
<li class="toctree-l1 current"><a class="current reference internal" href="#">Changes in KubeSpawner</a><ul>
<li class="toctree-l2"><a class="reference internal" href="#unreleased">Unreleased</a></li>
<li class="toctree-l2"><a class="reference internal" href="#id1">20</a><ul>
<li class="toctree-l3"><a class="reference internal" href="#id2">2.0.1 - 2022-02-15</a><ul>
<li class="toctree-l4"><a class="reference internal" href="#maintenance-and-upkeep-improvements">Maintenance and upkeep improvements</a></li>
<li class="toctree-l4"><a class="reference internal" href="#contributors-to-this-release">Contributors to this release</a></li>

Any suggestions?

@choldgraf
Copy link
Member

choldgraf commented Feb 24, 2022

I'm not sure what you mean - that output looks correct to me. What is the problem with it? Can you give an example of a page with incorrect behavior?

@rccern
Copy link
Contributor Author

rccern commented Feb 24, 2022

<li class="toctree-l2"><a class="reference internal" href="#id1">20</a><ul>

this line should be <li class="toctree-l2"><a class="reference internal" href="#20">20</a><ul>

or am i missing something?

@manics
Copy link
Member

manics commented Feb 24, 2022

In case you weren't aware readthedocs provides PR builds for testing.

E.g.
https://jupyterhub-kubespawner--575.org.readthedocs.build/en/575/spawner.html#kubespawner.KubeSpawner.consecutive_failure_limit
links directly to consecutive_failure_limit
and
https://jupyterhub-kubespawner--575.org.readthedocs.build/en/575/changelog.html#id27
links to the 0.16.1 changelog.

@rccern
Copy link
Contributor Author

rccern commented Feb 24, 2022

thank @manics , I noticed that, but maybe @choldgraf wanted to have https://jupyterhub-kubespawner--575.org.readthedocs.build/en/575/changelog.html#016 point to 0.16 (or did I misunderstand)? otherwise, the PR looks ok to merge

@choldgraf
Copy link
Member

choldgraf commented Feb 24, 2022

@rccern I suspect that id1 is used because starting an HTML ID with a number is not valid - my guess is that it does some kind of check for the right structure, and if it doesn't find it, then it generates an ID based on a generic structure. If you pre-pended v before each title (so v2.0 instead of just 2.0) maybe that'd work?

@rccern
Copy link
Contributor Author

rccern commented Feb 24, 2022

I checked https://html.spec.whatwg.org/multipage/dom.html#global-attributes:the-id-attribute-2 , in particular "There are no other restrictions on what form an ID can take; in particular, IDs can consist of just digits, start with a digit, start with an underscore, consist of just punctuation, etc.".

the v20 tricks works, btw

maybe something to check on MyST side? I'll unpin myst version

edit: docutils generates 'polyglot html' which means that is also xml, and "Polyglot markup ensures that every id attribute must be unique within the document and must be a legal XML name, starting with a letter." https://www.w3.org/TR/html-polyglot/#id-attribute

@choldgraf
Copy link
Member

Thanks for that analysis - I opened up a bug to track this here: executablebooks/MyST-Parser#527

@choldgraf
Copy link
Member

In the meantime, I think the easiest path forward is to add a letter to the beginning of the headers on this page (as well as in the references to it in the text)

@rccern
Copy link
Contributor Author

rccern commented Feb 25, 2022

i think it's up to @consideRatio to choose if they are really needed. I think the current status (https://jupyterhub-kubespawner--575.org.readthedocs.build/en/575/changelog.html#id27) is already an improvement and this PR can be already merged as is

@manics
Copy link
Member

manics commented Feb 25, 2022

We should check whether the #id27 anchors remains constant across updates to the changelog. E.g. If adding a new changelog entry results in the anchors changing this is a problem!

@rccern
Copy link
Contributor Author

rccern commented Feb 25, 2022

ah I didn't think about that.... then if you want i can add a commit that prepends v to all version, but I don't know if that will break any release automation

@consideRatio
Copy link
Member

@rccern, @choldgraf, @manics ❤️ 🎉 🌻 thank you so much for your investigative work into resolving this very thoroughly!!

I'll leave it to @manics / @choldgraf to merge given the better comprehension of the situation.

@manics
Copy link
Member

manics commented Feb 25, 2022

Looks like the #id[] is generated consecutively, so for example it's already incorrect in some of our existing released docs such as https://jupyterhub.readthedocs.io/en/stable/changelog.html#id1

Therefore I don't think adding a v prefix in this project only makes sense.

@choldgraf
Copy link
Member

choldgraf commented Feb 25, 2022

I'd recommend not relying on auto-generated labels, and instead manually specifying them only when you need them. So for example, you can generate labels like:

(mylabel)=
## My section

Reference [](mylabel) or [with some text](mylabel).

I find that this is less brittle and leads to fewer unexpected surprises anyway, because you specifically define the label rather than compute it from the section title (for example, it won't break if you change a word or letter in the section title).

@manics what do you mean by "consecutively"? I think that if the section started with a letter, then the id# pattern wouldn't be used at all. ah I see what you mean - yep if you added an extra header that starts w/ a number before all of the other ones, it would bump id# for them all.

If anybody wants to give a shot at a PR in the issue I linked above, that'd be nice as well :-)

@consideRatio consideRatio changed the title Fix broken anchors in docs by transitioning to MyST Replace recommonmark with myst_parser and fix changelog rendering Mar 10, 2022
@consideRatio consideRatio merged commit 88fd981 into jupyterhub:main Mar 10, 2022
@welcome
Copy link

welcome bot commented Mar 10, 2022

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@consideRatio
Copy link
Member

While this doesn't solve all relevant issues, it is great progress! I'm making a push towards getting a new release out and added a commit to unpin myst_parser and then went for a merge.

To resolve failing anchors in our Changelog where we have headings that are numerical, we should work on executablebooks/MyST-Parser#527.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide HTML #anchor tags for our configuration reference Readthedocs changelog anchors are broken
4 participants