jbms / sphinx-immaterial

Adaptation of the popular mkdocs-material material design theme to the sphinx documentation system
https://jbms.github.io/sphinx-immaterial/
Other
203 stars 32 forks source link

Please test with Docutils 0.19b1 #113

Closed AA-Turner closed 2 years ago

AA-Turner commented 2 years ago

Hi,

We've just released Docutils 0.19b1, and intend to release 0.19 final on 05/07/2022 (a week on Tuesday). Please would you test with the pre-release, and raise any issues to me either on this issue or on the Docutils tracker?

Thanks, Adam

2bndy5 commented 2 years ago

We're pinned to whatever version sphinx is pinned to. Currently that excludes any 0.19 versions. So, we have no reason to accommodate your request (unless Sphinx decides to update their current docutils requirements).

AA-Turner commented 2 years ago

Hi Brendan,

Sphinx uses Docutils' HTML writers, and we've made changes in the HTML writers in DU 19. Sphinx is also likely to declare support for DU 19 in Sphinx 5.1 (all being well) -- if the theme breaks because of changes, we'd like to know as early as possible so we can mitigate in Docutils.

Installing Docutils 0.19 will work with Sphinx 5.0.2, as we (Sphinx) test with Docutils master.

Even if a quick visual comparison of DU 18.1 vs DU 19b1, I would appreciate it if possible.

A

2bndy5 commented 2 years ago

Ah, ok. Well, we're a small dev group with other projects going, so I can't promise any feedback soon. This theme does some heavy monkey-patching of sphinx internals, so I'm not sure our feedback will be unblemished by this.

AA-Turner commented 2 years ago

No worries at all, completely understand!

As an aside, if there are changes you'd like to upstream to Sphinx I would be happy to look at these on Sphinx's repo.

A

2bndy5 commented 2 years ago

if there are changes you'd like to upstream to Sphinx I would be happy to look at these on Sphinx's repo.

Be careful what you wish for.

  1. There's still some unresolved issues from @jbms.
  2. Also @jbms PRs are getting stale.
  3. Sphinx v6.0 will break our theme unless we monkey-patch literal blocks with its current capability. I've voiced my concern at https://github.com/sphinx-doc/sphinx/issues/10265
AA-Turner commented 2 years ago

I don't know C++, so I'm not sure how much help I'd be on those issues (if I don't need to know C++ to review them though I am happy to, @jbms -- could you advise here please?).

I've commented on https://github.com/sphinx-doc/sphinx/issues/10265, I see the issue there -- hopefully we can reach a happy medium but if not reverting the removal is entirely an option.

I'll try and find time for the other issues, at the moment that is limited to weekends pretty much though, sorry.

A

2bndy5 commented 2 years ago

I just did a quick peek of our docs built with docutils 0.19b1, and I see no new errors that we aren't already aware of.

2bndy5 commented 2 years ago

I do get these warnings:

WARNING: extlinks: Sphinx-6.0 will require a caption string to contain exactly one '%s' and all other '%' need to be escaped as '%%'.
WARNING: extlinks: Sphinx-6.0 will require a caption string to contain exactly one '%s' and all other '%' need to be escaped as '%%'.
WARNING: extlinks: Sphinx-6.0 will require a caption string to contain exactly one '%s' and all other '%' need to be escaped as '%%'.

but I don't think that's related to docutils. Our docs' conf.py shouldn't be triggering these warnings though, so its kinda confusing. https://github.com/jbms/sphinx-immaterial/blob/96f6f08cbe7f2591f384c1c42d8b93ebdeb5039b/docs/conf.py#L169-L177 And I don't see the extlinks API used in our code base.


All use of extlinks extension is copied from the RST primer page in sphinx docs. So, if we need to update that, then it wouldn't be hard.

jbms commented 2 years ago

Note: I already fixed those extlinks warnings in #99.

2bndy5 commented 2 years ago

I fixed the warnings by changing the config.py to

extlinks = {
    "duref": (
        "http://docutils.sourceforge.net/docs/ref/rst/restructuredtext.html#%s",
        "%s reference",
    ),
    "durole": ("http://docutils.sourceforge.net/docs/ref/rst/roles.html#%s", "%s role"),
    "dudir": (
        "http://docutils.sourceforge.net/docs/ref/rst/directives.html#%s",
        "%s directive",
    ),
}

Apparently, the warning is triggered for empty caption strings.

2bndy5 commented 2 years ago

I don't see a cause for concern with docutils 0.19b1 (surprisingly). I'm more concerned with changes to sphinx itself.

Please re-open if there is a newer 0.19 beta to test (or start a new thread).

jbms commented 2 years ago

@AA-Turner Thanks for your offer to help.

As @2bndy5 mentioned, there are a bunch of C++-related Sphinx PRs that have been sitting for a few months now. I have pinged @jakobandersen a few times regarding them, but he indicated that unfortunately he doesn't have much time to review Sphinx PRs these days. I think the C/C++ domains in Sphinx could benefit from an additional maintainer, but indeed the code is rather complicated. I think my PRs are in fairly good shape already --- they have unit tests --- but I imagine there might be some things Jakob would do differently.

There are a number features in this theme that could be good candidates to upstream into Sphinx itself:

2bndy5 commented 2 years ago

I haven't been able to ping Jake for breathe contributions either - seems to be MIA for now.

If you'd like to open thread(s) for upstreaming the domain/obj_desc customizations, feel free. This theme is still technically in beta, so everything we monkey patch feels rather brittle until equivalent additions are made to Sphinx. Right now, the monkey patching has only broken the breathe extension (that we know of).