-
Notifications
You must be signed in to change notification settings - Fork 92
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
Rebase on new version of mkdocs-material #96
Conversation
Hello @jbms! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-09-25 00:20:35 UTC |
DeepCode failed to analyze this pull requestSomething went wrong despite trying multiple times, sorry about that. |
One other thing: I did not do this yet, but it may be useful to designate the relevant commit from mkdocs-material as an additional parent of this commit. That way it is easier to merge in later changes from there. However, the downside is that it will pull in the entire history of mkdocs-material, which is somewhat large due to having a lot of generated data and icon copies. Also, currently I have excluded the generated files from the repository. That does mean users have to use node.js to build the generated files when installing from the git repository, though. You could have a CI workflow that builds the generated files and pushes them to another branch --- that would avoid polluting the main branch but still allow users to install the latest version with needing node.js. |
Note: I'd suggest when reviewing this that you also look at the diff from mkdocs-material, since that is much smaller than the difference from the sphinx-material master. |
f7393f5
to
eadcf67
Compare
Thanks for this. It will take a while to get it all done. Is there any chance it would be better to have a submodule for mkdocs material with a specific commit? |
To make it easier to review the changes, I split my changes into several individual commits. Additionally, I added the upstream mkdocs-material commit on which these changes are based as a merge parent. Unfortunately that does mean this pull request shows every commit from mkdocs-material --- you can find my commits at the end, starting from "Pull non-excluded files from mkdocs-material/master". You can look at just those commits to see the changes I had to make, which are relatively minor except for the search integration. I think this approach will work reasonably well. The only downside is that your repository will now be at least as large as the mkdocs-material repository --- and that repository is somewhat large due to it containing a bunch of copied icons and generated javascript/css bundles that are re-generated on every commit. But I don't think there is a better way. I don't think git submodules would work that well, because some changes need to be made to the mkdocs-material files --- they at least some of them can't just be referenced without modification. Because it is slightly tricky to do the merging, I also added a |
1c441a7
to
b510b5f
Compare
Ooo! This is neat! @jbms you seem to have done most of same things as I did in https://github.com/pradyunsg/sphinx-mkdocs-theme, but significantly more thoroughly; albeit in a theme-specific manner! ^.^ |
@pradyunsg Interesting, hadn't seen that. Certainly there is an advantage in supporting any mkdocs theme rather than just mkdocs-material, though some theme-specific changes seem to be inherently necessary to account for mismatches between the Sphinx and mkdocs document models (e.g. mkdocs doesn't expect pages to have sub-pages in the toc), search support, and support for Sphinx "object descriptions" which don't exist in mkdocs. Currently I'm still working on revising this theme to add better styling for object descriptions, inspired by your own Furo theme and by pdoc3. I'm ultimately planning to use this theme for the documentation for https://github.com/google/tensorstore In general I very much like the power of sphinx, and it provides a lot of useful components, but a lot of assembly tends to be required to get a nice result... It would be great to avoid duplicated effort between this theme and your sphinx-mkdocs-theme extension, but I'm also not sure how feasible it is to accomplish what I want to accomplish with this theme with an additional layer underneath. |
Maybe? I was planning to enforce that as "nesting on the top level needs to be a captioned ToC". That realistically covers the whole toctree situation. :)
I got a very neat (and hacky) solution working for this: populate a Lunr.js index from the Sphinx documents! :)
I imagine this is the API documentation stuff? Yea, this one is tricky. This is basically why I abandoned this effort, since there was no longer a way to do things in a general way. :) |
On Wed, Mar 31, 2021, 22:35 Pradyun Gedam ***@***.***> wrote:
some theme-specific changes seem to be inherently necessary to account for
mismatches between the Sphinx and mkdocs document models (e.g. mkdocs
doesn't expect pages to have sub-pages in the toc)
Maybe? I was planning to enforce that as "nesting on the top level needs
to be a captioned ToC". That realistically covers the whole toctree
situation. :)
search support
I got a very neat (and hacky) solution working for this: populate a
Lunr.js index from the Sphinx documents! :
I looked into that approach a bit. I haven't done comparisons of the
actual searching effectiveness, but I like how the sphinx index works
better: the lunr index anyways contains the full text content of the entire
site, and if a pre built index is used is several times larger than that.
The sphinx index appears to be more compact (assuming most terms appear
multiple times in a document) since it only stores (docid, term)
associations. It also knows about object descriptions, so they can be
handled specially. The downside to the sphinx index is that you have
actually fetch pages in order to extract snippets and section information.
… object descriptions
I imagine this is the API documentation stuff? Yea, this one is tricky.
This is basically why I abandoned this effort, since there was no longer a
way to do things in a general way. :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#96 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABAEJ2QF4P7VWHLKPOWPAFDTGQA3PANCNFSM4YOKXORQ>
.
|
@jbms great to see work on this, I was just looking at Furo and this theme to refresh some Sphinx docs. Just another idea in case it works for you: our org is also basing a (mkdocs) theme on the upstream You can then update the submodule continuously using dependabot/renovate, and test that it still works against upstream, with smaller deltas that are probably easier to resolve. It should be much cleaner than vendoring all the code long-term (from a quick glance it seems like you've had to copy/paste a lot of upstream files that might not be relevant for you). Of course that's probably much easier to do with mkdocs-mkdocs customization than applying an mkdocs upstream theme to a sphinx theme. Just wanted to add this here in case you find it feasible and makes your life easier later down the road. But maybe this is more a topic for maintainers here (@bashtage?). |
As an update, my own work on this effort is now pushed out here, currently embedded within the tensorstore repository itself: You can see the generated documentation here to see the theme in action: In particular, refer to the API documentation section, which required the most significant sphinx customizations: The theme adheres to mkdocs-material extremely closely, but the API documentation styling is largely my own invention, inspired by pdoc3 (and https://hikari-py.github.io/hikari/hikari/), and other sphinx themes like Furo, as mkdocs-material does not itself offer anything specific to API documentation. I have kept up to date with upstream mkdocs-material. Currently the theme requires sphinx 3, but I also have some pending changes that I expect will be pushed out within a week or so to update to Sphinx 4 and to the latest mkdocs-material. I would love to be able to upstream these changes back into sphinx-material and/or sphinx itself as appropriate, but I'm not sure what the best way forward is. I can create an updated version of this pull request, but before spending more time on that it would be helpful to know if the direction I've taken things is one that @bashtage or others are interested in. |
Just my 2 cents: Your updates push the sphinx-material to a much better level, because it extend some needed functions and cut some useless. And I'm happy you continue your backports. :) Thanks! The only problem we had is the migration from this repo to your version. Maybe this is getting easier with your example documentation. |
FWIW, if @bashtage doesn't respond soon, it might be worth making this PR into a separate project that's effectively a better maintained fork of this project. |
Alternatively, if you really like the https://www.python.org/dev/peps/pep-0541/#continued-maintenance-of-an-abandoned-project |
Shoukd decide whether this should be a stand alone theme or if someone want to take up full time maintenance. This was always a side project for me |
@pradyunsg this is not an avoiding project. There was a release last week. This update changes a lot and I need a clean block of time to figure out if these are welcome changes for the intended use, e.g., statsmodels.org |
It sounds like there is definitely some interest in this theme, but it is unclear who has time to actually maintain it :) I think I can commit to keeping it synced with upstream mkdocs-material. (I just pushed to the tensorstore repository an update that makes it work with sphinx 4 and syncs with newer mkdocs-material changes.) However, I don't think I can really take on the additional package maintenance work of updating the documentation and examples, making releases, etc. Also it would be great to get some help trying to reduce the amount of monkey patching required by merging the necessary fixes into upstream sphinx (see this issue I filed with sphinx sphinx-doc/sphinx#9523). Finally, for the tensorstore documentation there are some additional customizations that currently are partially separated from the theme itself: https://github.com/google/tensorstore/tree/master/docs/tensorstore_sphinx_ext Most notably this includes a new implementation of autosummary and some monkey patching of autodoc in order to support pybind11 better. However, those changes aren't completely independent of the theme code --- for example there are some style rules in the theme that are only used by the new autosummary. Currently the tensorstore_sphinx_ext code does contain some tensorstore-specific stuff, but that could be factored out easily enough. However, I'm not clear on where that should go. Ideally some of it could go into upstream sphinx --- for example, the support for pybind11 overloaded functions. Potentially the new autosummary could live as part of sphinx-material. There is also a new json domain for documenting json schemas that integrates with some of the new theme functionality but is otherwise unrelated but might also be useful to some people. |
Comment from the peanut gallery: cannot speak for @bashtage , but I think this would be way more manageable if it was split into smaller pull requests. There are more than 3 000 commits and the diff says |
@jbms I'm blown away! The way you have styled your tensorstore website, with the new Material for Mkdocs theme and modified Sphinx autosummary, is just as I've always wanted. Especially the new autodoc/autosummary feature. I would love to see this finally get merged or break off into a standalone project. I'm trying to build my FOSS Python package's docs off of your fork. (Still working through a couple of npm issues, which are a little tricky because I'm not a web dev) @jbms, please don't give up on this. 🙏 |
raise RuntimeError('Could not run \'npm run %s\'.' % t) | ||
|
||
if res != 0: | ||
raise RuntimeError('failed to build sphinx-material package') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tried to integrate this modified branch into my project's docs/requirements.txt like so
git+https://github.com/jbms/sphinx-material
sphinx-copybutton
But running python -m pip install -r docs/requirements.txt
fails to build the sphinx-material theme. It's traceback ends on this line.
Furthermore, I did a fresh install of npm and retried... It failed again, but this time, npm reported
Error: Cannot find module 'fs/promises'
at Function.Module._resolveFilename (internal/modules/cjs/loader.js:636:15)
at Function.Module._load (internal/modules/cjs/loader.js:562:25)
at Module.require (internal/modules/cjs/loader.js:692:17)
at require (internal/modules/cjs/helpers.js:25:18)
at Object.<anonymous> (/tmp/pip-req-build-7y539i6_/tools/build/_/index.ts:24:1)
at Module._compile (internal/modules/cjs/loader.js:778:30)
at Module.m._compile (/tmp/pip-req-build-7y539i6_/node_modules/ts-node/src/index.ts:1056:23)
at Module._extensions..js (internal/modules/cjs/loader.js:789:10)
at Object.require.extensions.(anonymous function) [as .ts] (/tmp/pip-req-build-7y539i6_/node_modules/ts-node/src/index.ts:1059:12)
at Module.load (internal/modules/cjs/loader.js:653:32)
Is there a required npm (non-std) lib also? I did find this commit comment to be helpful, but I'm not well versed with node.js.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you may just need to update your version of nodejs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nodejs -v
returns v10.19.0. I'll have to look into how to update that. I installed npm via (WSL) ubuntu apt, so I might find more problems from that.
I wondor if this requirement might break compatibility with readthedocs.org... I wouldn't think so since it can host docs generated with mkdocs-material.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could look into it more, I was just guessing, but I was testing with nodejs version 14 I believe.
mkdocs-material does not require users of the package to have node.js because the author re-computes the generated files and checks them into the repository after every change.
That has a number of drawbacks. What I have setup instead in my branch is that if you build from the repository you will need nodejs because the generated file will be rebuilt automatically, but when you build a wheel (e.g. for publishing on pypi) the generated files will be pre-built. So users will still be able to pip install
without needing nodejs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I was able to update nodejs (from v10 to v16) via nvm, but I had to uninstall npm and nodejs via ubuntu apt first (it was problematic as I expected). I'm now able to install the the modified theme. Thank you for the assist!
I looked into readthedocs... Their docker image source seems to install npm and nodejs via apt 😦 which means this modified theme needs to be installed via pypi to avoid needing nodejs v14+. However, the RTFD manual suggests using a config file to ensure other software requirements are met, but I doubt that would help my situation (installing modified theme from git not pypi) due to the conflicts that installing nodejs from apt incur.
ugh. I'm now getting an error when building my docs with this modified theme:
it's so terse I was hoping @jbms might have some insight |
@2bndy5 I had not updated my branch in a while, and it did not support newer versions of sphinx. I just pushed out an updated version based on the latest changes in the tensorstore repository. It should now build with the latest sphinx and docutils. |
@jbms Thanks for the response (I hadn't considered my installed version of Sphinx which is v4.2.0). I just re-installed the theme from git and tried to re-build my docs, but hit this error:
It might be worth mentioning that my docs don't use auto-summary Assuming I can get this to work, I'm willing to throw my hat in the ring for possible maintainers... I'm just hesitant because I clearly don't know all the mechanics being used. I'm willing to learn as I go (one of the great opportunities in open-source contributions)! I mention this because I suspect this modified theme may need a home of its own (this way @bashtage isn't swamped) - we could call it sphinx-material2 or something. |
I just pushed an update that fixes that problem. |
@jbms yes indeed it did fix that problem. Thank you! My docs are now building locally. |
I should probably mention that I get an error on consecutive builds (I'm now playing with the theme's "features" 😃 )
deleting the built docs before re-building fixes this error. |
|
I'm not seeing the issue with code blocks scrolling horizontally. That seems to work for me, both in desktop Chrome when "mobile device mode" is selected in dev tools, and in firefox and chrome on android. I don't have a convenient way to test on iOS/Safari though. As far as the tables, the Regarding the failure with consecutive builds: for the tensorstore documentation it is set up to always do a clean build, so I hadn't noticed previously. The consecutive build error is related to the search index. One of my modifications was to strip out some unnecessary data included in the search index, that was not actually used for searching (and unnecessarily increased its size). Some other changes are also made to simplify the client-side searching. Refer to |
that's exactly what I'm doing:
Not sure how I would override the HTML translator, but I should mention that I'm using a custom pygment style for highlighting the code blocks (I never liked the default offerings). # pygment custom style
# --------------------------------------------------
class DarkPlus(Style):
"""A custom pygment highlighting scheme based on
VSCode's builtin `Dark Plus` theme"""
background_color = "#1E1E1E"
highlight_color = "#ff0000"
line_number_color = "#FCFCFC"
line_number_background_color = "#202020"
default_style = ""
styles = {
Text: "#FEFEFE",
Comment.Single: "#5E9955",
Comment.Multiline: "#5E9955",
Comment.Preproc: "#B369BF",
Other: "#FEFEFE",
Keyword: "#499CD6",
Keyword.Declaration: "#C586C0",
Keyword.Namespace: "#B369BF",
# Keyword.Pseudo: "#499CD6",
# Keyword.Reserved: "#499CD6",
Keyword.Type: "#48C999",
Name: "#FEFEFE",
Name.Builtin: "#EAEB82",
Name.Builtin.Pseudo: "#499DC7",
Name.Class: "#48C999",
Name.Decorator: "#EAEB82",
Name.Exception: "#48C999",
Name.Attribute: "#569CD6",
Name.Variable: " #9CDCFE",
Name.Variable.Magic: "#EAEB82",
Name.Function: "#EAEB82",
Name.Function.Magic: "#EAEB82",
Literal: "#AC4C1E",
String: "#B88451",
String.Escape: "#DEA868",
String.Affix: "#499DC7",
Number: "#B3D495",
Operator: "#FEFEFE",
Operator.Word: "#499DC7",
Generic.Output: "#F4DA8B",
Generic.Prompt: "#99FFA2",
Generic.Traceback: "#FF0909",
Generic.Error: "#FF0909",
Punctuation: "#FEFEFE",
}
def pygments_monkeypatch_style(mod_name, cls):
"""function to inject a custom pygment style"""
cls_name = cls.__name__
mod = type(__import__("os"))(mod_name)
setattr(mod, cls_name, cls)
setattr(pygments.styles, mod_name, mod)
sys.modules["pygments.styles." + mod_name] = mod
pygments.styles.STYLE_MAP[mod_name] = mod_name + "::" + cls_name
pygments_monkeypatch_style("dark_plus", DarkPlus)
# The name of the Pygments (syntax highlighting) style to use.
pygments_style = "dark_plus" Besides that, I did disable adding any extra css in conf.py |
I also noticed the toc is cut short on every rst file (cross-linking still works though) .. examples.rst
chapter 1
===============
section a
------------
paragraph 1
************
section b
-------------
.. gets cut off here
chapter 2
================== |
commented out all that custom pygment stuff and rebuilt... code-blocks still don't have horizontal scrolling and the code-block bg is statically set, but adding the following fixed the bg problem .md-typeset pre {
background-color: var(--md-code-bg-color);
} |
Regarding your TOC issue, it looks like you have multiple top-level headings in that file. The TOC code assumes a single page title, e.g.: My Title
=====
Chapter 1
------------
Chapter 2
----------- How would it work if there are multiple top-level headings? As far as pygments, mkdocs-material supplies its own pygments style, and so I have disabled the normal pygments css. Did you custom pygments styles work? Can you explain what the background color issue is? Also, can you reproduce those issues when you build the docs directly from the branch this pull request is based on? Because when I build the docs from this branch, the code blocks scroll horizontally, and tables have the |
Also, I'm thinking of following your suggestion and creating a fork for this version under a different name to avoid conflict with bashtage's package. I'm thinking of using the name |
This examples.rst file's toc renders (on the right side of the page) like so:
Yes, it worked like a charm, but I'm willing to forgo that bit in the name of properly reviewing this.
|
If you want to inspect the repo I'm using, I'm testing locally on my nRF24/CircuitPython_nRF24L01 repo (latest docs changes are hosted on RTFD.io). So, nothing regarding this modified theme has been commit. |
Regarding the TOC issue, I see that in your However, I think Sphinx generally is designed around the assumption that there is a single top-level heading per page. For example, that is used as the title of the page; in your case, your examples document ends up with a title of the first heading, "nRF24L01 Features", but I would suppose that "Examples" would be a more suitable page title. While it would probably be possible to fix my branch to support multiple top-level headings in some way, I think it would be simpler to just say that there has to be a single top-level heading. You could modify your document to be like: Examples
=======
nRF24L01 Features
----------------------
Library-specific Features
---------------------------
|
I'll look into a fix for your fork. This tactic I'm using worked for all other themes I tried (including RTD theme). Yes, I should change the name of the top-level heading because of the material's theme-specific usage of the first heading. It renders fine in the navbar (with and without the
I sincerly beg to differ.
|
Even in other themes, sphinx uses the first heading as the Maybe it makes sense to support multiple top-level headings per page. It is just that the existing TOC logic is already quite complicated, and making it work with the mkdocs-material templates is also complicated because mkdocs has a somewhat different TOC model compared to Sphinx. I will try to get the sphinx-immaterial repo set up tomorrow, and would certainly welcome your help as a maintainer. |
I have set up the sphinx-immaterial repository: https://github.com/jbms/sphinx-immaterial The documentation is mostly still unchanged from this repository and needs a lot of work, though. Also the new autosummary used for the tensorstore documentation is not yet integrated, and the theme does not work that well with the normal autosummary. It also doesn't work that well yet with numpydoc (styling is a bit weird). |
This could probably be closed as it is now its own standalone theme. Unless, the intention is to attract attention to the sphinx-immaterial theme - this theme's landing page could use a mention/link for that though. |
This is rebased on version 5bda328 of mkdocs-material (master as of 2021-03-01).
TODO:
Fixes #90 (I think)
Fixes #89
Fixes #44
Fixes #71
Fixes #92