customized admonition

[2bndy5]

solves #183 and conforms #182 with improvements used here.

[mhostetter]

Minor suggestion: perhaps instead of having the user instantiate a CustomAdmonition object in the sphinx_immaterial_custom_admonitions list in conf.py, maybe have the user specify a dictionary of kwargs. Those kwargs can be used internally to instantiate the CustomAdmonition objects. Might be a little more user-friendly.

From

from sphinx_immaterial.custom_admonitions import CustomAdmonition

sphinx_immaterial_custom_admonitions = [
    CustomAdmonition(
        name="example-admonition",
        color=(198, 177, 6),
        icon="material/school",
    ),
]

To

sphinx_immaterial_custom_admonitions = [
    {
        "name": "example-admonition",
        "color": (198, 177, 6),
        "icon": "material/school",
    },
]

[2bndy5]

I had considered it, but using dicts are problematic because undetected typos in the keys may result in its value not being used (& unused keys not noted during build time). We had worked around this in the past with the object_description_options config, but that is vastly more general of a configuration.

The other reason I didn't use a dict is because a class constructor can fill in unspecified default values for free (kwargs in c'tor). Plus, I can do type checking on the values (like making sure the color is rgb() friendly in CSS) before going any further with the docs build.

I'm not shooting it down, it should be possible, but I just found it quicker to get up and running with the class approach. Under the hood, it is still quite flexible to make a switch. I'll try it out in the coming days (still refining/testing the admonitions that override existing directives in sphinx).

[2bndy5]

I have been rather annoyed by a problem in our docs which always shows the __new__() function in the docs for the CustomAdmonition class (despite explicitly stating no-special-members and no-undoc-members). I think its a side effect of using autodoc and the python apigen in the same project.

[jbms]

I also like Matt's suggestion. If you use pydantic to define a data class and to parse the option in config_inited, that will take care of type checking and defaults.

[2bndy5]

Will do. I'm just finishing up the todo directive override now.

[2bndy5]

So switching to pydantic was easy, but now I'm trying to document the config values' datatypes... Currently trying to follow what was done for the obj_desc_opts, but it is complex. So far, I've created the new admoniconf directive/role and moved the params' description into the admonition.rst doc.

[jbms]

So switching to pydantic was easy, but now I'm trying to document the config values' datatypes... Currently trying to follow what was done for the obj_desc_opts, but it is complex. So far, I've created the new admoniconf directive/role and moved the params' description into the admonition.rst doc.

Does autodoc not work to document the dataclass?

[2bndy5]

Oh, I hadn't thought to do it that way. I was trying to move away from autodoc because the rogue __new__() that I can't seem to get rid of.

[jbms]

Oh, I hadn't thought to do it that way. I was trying to move away from autodoc because the rogue __new__() that I can't seem to get rid of.

How about adding it to :exclude-members:, as done here:

https://github.com/jbms/sphinx-immaterial/blob/main/docs/apidoc/cpp/apigen.rst

[2bndy5]

Opening this up for review.

Some changes to the generic admonitions should bring compliance to the deprecated aliased admonition CSS classes in upstream. Some of the classes getting removed in upstream v9.0 will have to be added back as Sphinx already has specific admonition directives that upstream CSS won't style. Namely, these classes are

• error
• caution
• attention
• hint
• important
• seealso
• todo

On the plus side, this will give us a chance to style them more uniquely (if desired). The main reason these CSS classes are deprecated is to cut back on the CSS bundle's footprint, but mkdocs isn't constrained by predefined admonitions like we are.

[2bndy5]

I think we need to reduce the workflow CI triggers. It is excessive to have the workflow run 2x for every commit in an open PR. I'm running into bandwidth/connection problems and it takes a pretty long time to complete 1x workflow run.

I've gotten more comfortable using pre-commit, which we can use to reduce linting errors reported in CI, cache dependencies on the github runner, trim trailing whitespace, detect yaml/json syntax errors, and ensure uniform line endings.

FYI, the coverage pkg's run command can invoke pytest without installing the theme src (& build a coverage report if that sounds interesting), and conf.py can be configured to add the theme src to sys.path (I've been doing this locally) -- resulting in less need to install the theme in CI.

[mhostetter]

I think we need to reduce the workflow CI triggers. It is excessive to have the workflow run 2x for every commit in an open PR.

I think if you change

sphinx-immaterial/.github/workflows/build.yml

Line 3 in 09b41f9

on: [push, pull_request, workflow_dispatch]

to

on:
  push:
    branches:
      - main
  pull_request:
    branches:
      - main
  workflow_dispatch:

that should do the trick.

[2bndy5]

not a fan of limiting by branch because we dev on any branch but main. I was thinking more like limiting the PR sync events:

on:
  push:
  pull_request:
    types: [opened, closed, repoened]
  workflow_dispatch:

[jbms]

Looks good to me, thanks for all your work on this!