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

Support theme-namespaced plugin loading #2998

Merged
merged 2 commits into from Oct 12, 2022
Merged

Support theme-namespaced plugin loading #2998

merged 2 commits into from Oct 12, 2022

Conversation

oprypin
Copy link
Contributor

@oprypin oprypin commented Oct 8, 2022

This is mainly aimed at 'material' theme which also ships plugins with it. It will be able to ship plugins under the name e.g. 'material/search' and that will ensure the following effects:

  • If the current theme is 'material', the plugin 'material/search' will always be preferred over 'search'.
  • If the current theme isn't 'material', the only way to use this plugin is by specifying plugins: [material/search].

One can also specify plugins: ['/search'] instead of plugins: ['search'] to definitely avoid the theme-namespaced plugin.

Previously:

@squidfunk

This is mainly aimed at 'material' theme which also ships plugins with it.
It will be able to ship plugins under the name e.g. 'material:search' and it will ensure the following effects:

* If the current theme is 'material', the plugin 'material:search' will always be preferred over 'search'.
* If the current theme *isn't* 'material', the only way to use this plugin is by specifying `plugins: [material:search]`.

One can also specify `plugins: [':search']` instead of `plugins: ['search']` to avoid the theme-namespaced plugin.
@oprypin oprypin added this to the 1.4.1 milestone Oct 8, 2022
@squidfunk
Copy link
Contributor

squidfunk commented Oct 9, 2022

Awesome, thanks for taking the time implementing this! One minor thought: is : a good separator? Since : has meaning in YAML, users always need to enclose plugins in quotes, or the YAML will not be valid. Have you thought about using . or another separator?

@oprypin
Copy link
Contributor Author

oprypin commented Oct 9, 2022

I felt like . is misleading as a separator because it's strongly associated with "child of a module" but this is "child of a theme" in a totally separate namespace.
Then nothing else than : came to mind. I think it works OK even in YAML but yes could be misleading there.
What other separator could it be?

@squidfunk
Copy link
Contributor

I agree with your concerns. Just wanted to bring it up before it's out in the world. I guess most users won't use the : syntax anyway but rely on the defaults, so it's probably not that much of a problem. Not easy to find a better separator. >, < and | are all part of YAML syntax and . suffers from the problem you mentioned. So I guess : is really the best choice.

@squidfunk
Copy link
Contributor

... or maybe material/search and /search?

@oprypin
Copy link
Contributor Author

oprypin commented Oct 9, 2022

/ is a great idea, I'll probably change to that.

Copy link
Member

@ultrabug ultrabug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool move

@oprypin oprypin changed the base branch from plugconf to master October 12, 2022 01:52
@oprypin oprypin merged commit 6fca6b5 into master Oct 12, 2022
@oprypin oprypin deleted the them branch October 12, 2022 01:54
@unverbuggt
Copy link

unverbuggt commented Nov 8, 2022

Hi,

just stumbled upon this merge while researching why the search plugin of material theme is always used if material was installed. And there is currently no way to use the default search plugin (only by uninstalling mkdocs-material).

As mentioned in this merge there can be only one plugin named "search" after get_plugins() method and the merge defines the least priority to the "mkdocs.contrib." one.

In my opinion this is unwanted behaviour because you could install and use different themes and plugins in the same python installation. The built "search_index.json" won't differ much between the material and mkdocs search plugin, but if one installs the insiders version of material it is different (more slim, but contains html tags)

So this merge provides a slim solution for the problem, but the merge alone won't solve it.

We also need to change the way get_plugins() in plugins.py builds the plugin dictionary. Here is my quick (and probably not very elegant) fix for "plugins.py":

@@ -55,10 +55,8 @@
     # Allow third-party plugins to override core plugins
     pluginmap = {}
     for plugin in plugins:
-        if plugin.name in pluginmap and plugin.value.startswith("mkdocs.contrib."):
-            continue
-
-        pluginmap[plugin.name] = plugin
+        name = plugin.value.split('.',1)[0] + '/' + plugin.name
+        pluginmap[name] = plugin
 
     return pluginmap

After the patch I get the behaviour described in the merge: If theme is not material then default search plugin is used, however if I wanted to use material-theme's search I'd specify it as "material/search"

@oprypin
Copy link
Contributor Author

oprypin commented Nov 8, 2022

Hi.
The solution is actually complete, just that the plugins themselves need to cooperate.
mkdocs-material will just need to rename its plugin from 'search' to 'material/search'.
https://github.com/squidfunk/mkdocs-material/blob/018e5f813bf28e2dc79879538e3cda90cd5b8ae5/pyproject.toml#L50
The developer has agreed to do so, but hasn't made the switch yet. The switch has been made in mkdocs-material-insiders, though.

@squidfunk
Copy link
Contributor

The PR with the necessary fix is ready to be merged, but we're waiting for the next funding goal to be hit, as this will imply a major version release. In the meantime, please use a virtual environment to isolate dependencies.

unverbuggt added a commit to unverbuggt/mkdocs-encryptcontent-plugin that referenced this pull request Nov 11, 2022
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

4 participants