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

refactor: Theme-agnostic code highlighting, respecting configs #202

Merged
merged 4 commits into from
Feb 4, 2021

Conversation

oprypin
Copy link
Member

@oprypin oprypin commented Dec 26, 2020

Infer the highlighter configs from what the current Markdown config is, trying to mimick the two most typical extensions and how they'd highlight a code block in Markdown normally.

This improves theme support, as previously only some assumptions based on each theme's defaults were supported, e.g. the readthedocs theme is assumed to rely only on JS highlighting, when in reality you just need to add a Markdown syntax highlight extension and you can use Pygments.

I had commented on this here: #159 (comment)

Another improvement stemming from this is support for disabling syntax highlighting altogether. (use_pygments: false). For that I also had to edit the template for function signature, to avoid all newlines in it, because previously inline highlighting relied on processing of the syntax highlighter to get rid of those.

Note that, according to the basic logic of this change (inferring highlight config from Markdown config), if there is no highlight extension selected, this one should disable highlighting as well. But I decided to enable highlighting by defauly, unless use_pygments: False is specified, because even mkdocstrings repo itself doesn't explicitly specify a highlighter, and would "regress".

And personal note: I don't really need this to make it into the upcoming release, would prefer it to not block the release.

@oprypin oprypin force-pushed the hlmd branch 5 times, most recently from 6862725 to f84413d Compare January 1, 2021 12:31
Infer the highlighter configs from what the current Markdown config is, trying to mimick the two most typical extensions and how they'd highlight a code block in Markdown normally.

This improves theme support, as previously only some assumptions based on each theme's defaults were supported, e.g. the readthedocs theme is assumed to rely only on JS highlighting, when in reality you just need to add a Markdown syntax highlight extension and you can use Pygments.

Another improvement stemming from this is support for disabling syntax highlighting altogether. (`use_pygments: false`). For that I also had to edit the template for function signature, to avoid all newlines in it, because previously inline highlighting relied on processing of the syntax highlighter to get rid of those.
@oprypin
Copy link
Member Author

oprypin commented Jan 3, 2021

Regressions run

Note that I intentionally ignore changes from <div class="highlight"> to <div class="codehilite">, of which there are very many. But you can see them in an older run.

I looked through top repos to find whether anyone relied on this "class highlight", and found these:

  1. https://github.com/arogozhnikov/einops/blob/ef7af8966e9a305290d06e9bd913f264cd78c6cf/docs/css/mkdocs.css#L1
    But actually there's almost no difference from this CSS on the site so even that one is OK.
  2. https://github.com/up42/up42-py/blob/ce3941016d62e3453c0099f4641488783eef2d39/docs/stylesheets/extra.css#L76 - doesn't seem major but haven't checked

Why the change:

If the site uses the codehilite extension, it creates <div class="codehilite"> (unless configured otherwise). But mkdocstrings creates <div class="highlight"> always. So these sites used to have 2 different css classes depending on the origin. Now that will always converge into 1, because I adhere to the config.

@pawamoy
Copy link
Member

pawamoy commented Jan 3, 2021

OK, LGTM. The change could be surprising in some repos indeed, but I feel it's a more correct approach, and if a change is needed on the user side it should be fairly small, like changing .highlight into .codehilite in CSS files or similar. This will need to be documented in the changelog of course (I'll take care of it - I don't think it should be marked as "breaking" change, what do you think?)

@oprypin
Copy link
Member Author

oprypin commented Jan 3, 2021

Good to hear! (probably don't merge yet)

@oprypin
Copy link
Member Author

oprypin commented Jan 3, 2021

I have no other comment on whether the change is breaking. Don't know your criteria.

@pawamoy pawamoy merged commit f9ea009 into mkdocstrings:master Feb 4, 2021
@pawamoy pawamoy mentioned this pull request Feb 4, 2021
4 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.

None yet

2 participants