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

Use toc_tokens to generate the TOC #1970

Merged
merged 1 commit into from Feb 17, 2020
Merged

Use toc_tokens to generate the TOC #1970

merged 1 commit into from Feb 17, 2020

Conversation

@jimporter
Copy link
Contributor

jimporter commented Jan 30, 2020

This needs more testing, but once it's done, this should resolve both #1910 and #770. I've tried to maintain backward compatibility here, though maybe it makes sense to change the meaning of level. Currently, it's 0-indexed, but it might be better to make it 1-indexed so that the level matches N in <hN> for the headers.

Copy link
Member

waylan left a comment

This is a great start. I've noted a few things below.

mkdocs/themes/mkdocs/mkdocs_theme.yml Show resolved Hide resolved
mkdocs/structure/toc.py Outdated Show resolved Hide resolved
mkdocs/structure/toc.py Outdated Show resolved Hide resolved
@jimporter

This comment has been minimized.

Copy link
Contributor Author

jimporter commented Jan 30, 2020

While working on this, I noticed that Python-Markdown doesn't correctly un-stash HTML for .toc_tokens. I filed that as Python-Markdown/markdown#899. Once that gets fixed, I'll finish this patch up.

@jimporter jimporter force-pushed the jimporter:toc-tokens branch 2 times, most recently from aeb5dac to 9914fb1 Jan 30, 2020
@waylan

This comment has been minimized.

Copy link
Member

waylan commented Jan 30, 2020

While working on this, I noticed that Python-Markdown doesn't correctly un-stash HTML for .toc_tokens. I filed that as Python-Markdown/markdown#899. Once that gets fixed, I'll finish this patch up.

This means that we will need to require the most recent version of Markdown. Traditionally we have supported much older Markdown versions. I wonder if we should leave the old HTML to TOC parser as a fallback. If the new version of Markdown is installed, use the toc_tokens, but if an older version is installed, pass the HTML toc through the _parse_html_table_of_contents function and use the output of that instead. We might even issue a DeprecationWarning informing the user to upgrade their version of Markdown.

On the other hand, we could just update the required version in setup.py, which would force Markdown to be updated on install.

One or the other needs to happen. Either way, it should be part of this PR. Let's see what happens with the Markdown fix first.

@jimporter

This comment has been minimized.

Copy link
Contributor Author

jimporter commented Jan 30, 2020

This means that we will need to require the most recent version of Markdown.

A third option would be to manually strip out the stashed HTML placeholders from toc_tokens if Python-Markdown isn't up-to-date. That should be equivalent to what the code does with toc, and would be more similar to the proper way using the most recent (future) version of Markdown.

@jimporter jimporter closed this Feb 2, 2020
@jimporter jimporter deleted the jimporter:toc-tokens branch Feb 2, 2020
@waylan

This comment has been minimized.

Copy link
Member

waylan commented Feb 2, 2020

Did you close this intentionally? I thought this was a good start

Sent with GitHawk

@jimporter

This comment has been minimized.

Copy link
Contributor Author

jimporter commented Feb 2, 2020

Whoops, I think I unintentionally deleted this branch when I was cleaning up my repo. Let's see if I can undo that...

@jimporter jimporter restored the jimporter:toc-tokens branch Feb 2, 2020
@jimporter jimporter reopened this Feb 2, 2020
@waylan

This comment has been minimized.

Copy link
Member

waylan commented Feb 7, 2020

FYI, Markdown 3.2 has just been released, which includes the fix this is waiting for (Python-Markdown/markdown#901).

@jimporter

This comment has been minimized.

Copy link
Contributor Author

jimporter commented Feb 7, 2020

Cool, I'll work on finishing this up, hopefully by the end of the weekend. @waylan, if you're still concerned about requiring the latest version of Python-Markdown in MkDocs, I could work around that by stripping the stashed HTML placeholders from the TOC object. It should just be a simple regex. I'd prefer (not strongly) to just require Python-Markdown 3.2 though.

@waylan

This comment has been minimized.

Copy link
Member

waylan commented Feb 7, 2020

Requiring Markdown 3.2 is fine. As it turns out, the upcoming release of MkDocs and Markdown 3.2 are both the first versions to drop support for Python 2.7, so it makes sense that MkDocs updates it's dependency anyway.

@jimporter jimporter force-pushed the jimporter:toc-tokens branch from 9914fb1 to cc71596 Feb 7, 2020
@jimporter

This comment has been minimized.

Copy link
Contributor Author

jimporter commented Feb 7, 2020

@waylan Ok, this is mostly good. I did notice one weird thing that's causing a test failure though: if you have a header like # foo > &quot;, Python-Markdown 3.2 returns that as 'foo > &quot;' in .toc_tokens (3.1.1 returns 'foo > \x02wzxhzdk:0\x03' because it's not unstashing the HTML). On the other hand, the HTML in .toc would be foo &gt; &quot;, which is easier for end users to work with. This makes it a bit tough to escape the text properly for use in the templates, since we'd have to use Markdown's fancy HTML escaping logic.

This seems like a bug in Python-Markdown, since it does strip Markdown inside the header for .toc_tokens (i.e. # foo *bar* becomes 'foo bar'). This case wasn't previously covered in the Python-Markdown tests, so it never even occurred to me that this is what we'd end up with until I went to finish this PR... :/

@waylan

This comment has been minimized.

Copy link
Member

waylan commented Feb 7, 2020

This seems like a bug in Python-Markdown

Yes, it is. I've reported it at Python-Markdown/markdown#906.

@waylan

This comment has been minimized.

Copy link
Member

waylan commented Feb 12, 2020

Python-Markdown 3.2.1 was just released with a fix for the escaping issue. That needs to be the minimum version supported by MkDocs now.

setup.py Outdated Show resolved Hide resolved
@jimporter jimporter force-pushed the jimporter:toc-tokens branch 2 times, most recently from d36347e to f8c9343 Feb 13, 2020
This patch improves the consistency of TOC levels, so now the level is always
equal to the N in the `<hN>` tag. It also allows users of the MkDocs theme to
set the navigation depth to show in the TOC panel (defaulting to 2).
@jimporter jimporter force-pushed the jimporter:toc-tokens branch from f8c9343 to 71db354 Feb 13, 2020
@jimporter jimporter changed the title [WIP] Use toc_tokens to generate the TOC Use toc_tokens to generate the TOC Feb 13, 2020
@jimporter jimporter requested a review from waylan Feb 13, 2020
@waylan
waylan approved these changes Feb 17, 2020
@waylan

This comment has been minimized.

Copy link
Member

waylan commented Feb 17, 2020

@jimporter this looks good. Awesome job. I just want to make sure the change to have TOC items being 1-indexed is not going to be a problem for third party themes. @squidfunk do you have any input on this? The idea is that the TOC level attribute will now always match n in Hn for all headers (H1-H6). I don't intend to change the behavior back, but should we be advertising this as a backward incompatible change?

@squidfunk

This comment has been minimized.

Copy link

squidfunk commented Feb 17, 2020

@waylan it’s been a long time but I have a hack in place that detects whether there is an h1 in the content of the page to otherwise use the page title as h1, because as I recall level didn’t provide that information as it was zero-indexed. It effectively meant that I could not use this information, as it was always relative to the lowest hn. Is that what you mean? If so, it would not be a backward incompatible change, because it’s more of a feature, since we now have something that we didn’t have before. I could finally get rid of that hack.

@waylan

This comment has been minimized.

Copy link
Member

waylan commented Feb 17, 2020

@squidfunk thanks for the feedback. And we're glad to provide a useful feature.

@waylan waylan merged commit 37e645d into mkdocs:master Feb 17, 2020
3 checks passed
3 checks passed
codecov/project 91.23% (target 90.00%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jimporter jimporter deleted the jimporter:toc-tokens branch Feb 21, 2020
jimporter added a commit to jimporter/mkdocs-bootswatch that referenced this pull request Feb 21, 2020
waylan added a commit to mkdocs/mkdocs-bootswatch that referenced this pull request Feb 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.