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]

How about adding it to :exclude-members:

I didn't see that in the autodocs' docs. It worked!

It doesn't show the default value for the class attrs, but I can use the __init__ params instead.

[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

https://github.com/jbms/sphinx-immaterial/blob/09b41f950db5b3ba62143aca14276df6c6f3fcc8/.github/workflows/build.yml#L3

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:

[mhostetter]

not a fan of limiting by branch because we dev on any branch but main.

To be clear, my suggestion would run the CI on push to any branch that is tied to a PR to be merged onto main and anytime someone pushes to main (either manually or post-PR merge).

[2bndy5]

I hadn't realized the PR target was the determining factor for trigger filters. That sounds like its worth a try.

[jbms]

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.

We could support aliases by remapping them in Python to a different CSS class, so that there is no need for additional CSS.

We could also keep track of which ones are used and only add the CSS as needed.

[2bndy5]

We could support aliases by remapping them in Python to a different CSS class, so that there is no need for additional CSS.

We could also keep track of which ones are used and only add the CSS as needed.

Using the aliases in python is certainly easier. We wouldn't need to add CSS or keep track of the admonitions used if we just change the default class used for the aliased admonitions. It would be similar to how I managed using a different name in the inherited admonition directives and still use the upstream CSS classes (currently not using the deprecated class aliases specific to mkdocs-material though).

[2bndy5]

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

We could support aliases by remapping them in Python to a different CSS class, so that there is no need for additional CSS.

We are now prepared for this, but I left the code commented out until it is needed.

https://github.com/jbms/sphinx-immaterial/blob/70a25e57156f1137fd9361ba4eace43223c500e6/sphinx_immaterial/custom_admonitions.py#L211-L219

[jbms]

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