Repository navigation
MM Uses Plugins (Don't Merge Yet) - #396
Merged
Merged
Conversation
Refactor default submodule insertion to use try_emplace for better error handling. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Clarify operator== documentation to specify comparison with rhs. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Corrected a typo in the documentation regarding plugin initialization. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
# Conflicts: # cxx/include/pluginplay/module_manager/module_manager.hpp # cxx/include/pluginplay/module_manager/module_manager_class.hpp # cxx/include/pluginplay/module_manager/module_map.hpp # cxx/include/pluginplay/plugin/plugin.hpp # cxx/include/pluginplay/plugin/plugin_class.hpp # cxx/src/pluginplay/module_manager/detail_/module_manager_pimpl.hpp # cxx/src/pluginplay/module_manager/detail_/module_manager_pimpl.ipp # cxx/src/pluginplay/module_manager/export_module_manager.hpp # cxx/src/pluginplay/module_manager/export_module_map.cpp # cxx/src/pluginplay/module_manager/module_manager_class.cpp # cxx/src/pluginplay/module_manager/module_map.cpp # cxx/src/pluginplay/plugin/export_plugin_class.cpp # cxx/src/pluginplay/plugin/plugin_class.cpp # cxx/src/pluginplay/plugin/py_plugin.hpp # docs/source/developer/design/pluginplay_initialization.rst # docs/source/developer/design/plugins.rst # tests/cxx/unit_tests/pluginplay/examples/load_modules.hpp # tests/cxx/unit_tests/pluginplay/module_manager/detail_/module_manager_pimpl.cpp # tests/cxx/unit_tests/pluginplay/module_manager/module_map.cpp # tests/cxx/unit_tests/pluginplay/plugin/plugin_class.cpp # tests/python/unit_tests/module_manager/test_module_map.py
ryanmrichard
requested review from
jwaldrop107
and
a balanced review from Copilot
October 14, 2026 14:35
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Cache isolation, Python handle lifetime, module attachment, and documentation path handling have unresolved correctness issues.
5 open findings
What changed in this PR
Extends the Plugin design from #395 into ModuleManager, replacing free-function plugin loading. Downstream plugins must migrate before merging.
Changes:
- Adds plugin ownership, qualified module lookup, and cross-plugin defaults.
- Updates Python bindings, runtime/cache attachment, and documentation generation.
- Migrates examples, tutorials, and tests to plugin-based loading.
| File | Description |
|---|---|
| tests/python/unit_tests/module_manager/test_qualified_key.py | Tests qualified keys. |
| tests/python/unit_tests/module_manager/test_module_map.py | Tests map mutations. |
| tests/python/unit_tests/module_manager/test_module_manager_class.py | Tests plugin loading and defaults. |
| tests/python/doc_snippets/test_modules.py | Tests plugin ownership transfer. |
| tests/python/doc_snippets/export_pluginplay_examples.cpp | Exports the example plugin class. |
| tests/cxx/unit_tests/pluginplay/printing/document_modules.cpp | Tests plugin documentation directories. |
| tests/cxx/unit_tests/pluginplay/plugin/plugin_class.cpp | Tests case-insensitive provider names. |
| tests/cxx/unit_tests/pluginplay/module_manager/qualified_key.cpp | Tests qualified-key semantics. |
| tests/cxx/unit_tests/pluginplay/module_manager/module_map.cpp | Tests copying and mutations. |
| tests/cxx/unit_tests/pluginplay/module_manager/module_manager_class.cpp | Tests plugin-aware manager operations. |
| tests/cxx/unit_tests/pluginplay/module_manager/detail_/module_manager_pimpl.cpp | Tests attachment and plugin state. |
| tests/cxx/unit_tests/pluginplay/examples/writing_a_plugin.hpp | Adds a plugin tutorial example. |
| tests/cxx/unit_tests/pluginplay/examples/workflow.hpp | Migrates workflow initialization. |
| tests/cxx/unit_tests/pluginplay/examples/workflow.cpp | Tests plugin-based loading. |
| tests/cxx/unit_tests/pluginplay/examples/load_modules.hpp | Removes the old loading example. |
| tests/cxx/unit_tests/pluginplay/examples/lambda_module.cpp | Loads the example plugin. |
| tests/cxx/doc_snippets/test_tutorial_module_documents.cpp | Checks plugin-scoped documentation output. |
| tests/cxx/doc_snippets/test_modules.cpp | Migrates tutorial module setup. |
| tests/cxx/doc_snippets/pluginplay_examples.hpp | Declares the example plugin. |
| tests/cxx/doc_snippets/pluginplay_examples.cpp | Implements the example plugin constructor. |
| tests/cxx/doc_snippets/generate_module_documentation.cpp | Loads a plugin before generating documentation. |
| docs/source/tutorials/plugins/writing_a_plugin.rst | Adds plugin authoring guidance. |
| docs/source/tutorials/plugins/overview.rst | Updates plugin layout guidance. |
| docs/source/tutorials/plugins/load_modules.rst | Removes obsolete loading guidance. |
| docs/source/tutorials/plugins/index.rst | Links the replacement tutorial. |
| docs/source/tutorials/plugins/document_modules.rst | Documents plugin-based generation. |
| docs/source/developer/design/plugins.rst | Updates plugin ownership and macro design. |
| docs/source/developer/design/pluginplay_initialization.rst | Describes lookup, caches, and defaults. |
| cxx/src/pluginplay/printing/mermaid.cpp | Traverses submodules directly. |
| cxx/src/pluginplay/printing/document_modules.cpp | Groups documentation by plugin. |
| cxx/src/pluginplay/plugin/py_plugin.hpp | Preserves Python plugin lifetime. |
| cxx/src/pluginplay/plugin/plugin_class.cpp | Matches provider names case-insensitively. |
| cxx/src/pluginplay/plugin/export_plugin_class.cpp | Uses smart-holder ownership. |
| cxx/src/pluginplay/module/detail_/module_pimpl.ipp | Implements runtime and cache attachment. |
| cxx/src/pluginplay/module/detail_/module_pimpl.hpp | Declares attachment support. |
| cxx/src/pluginplay/module_manager/module_map.cpp | Implements module insertion and copying. |
| cxx/src/pluginplay/module_manager/module_manager_class.cpp | Delegates plugin-aware operations. |
| cxx/src/pluginplay/module_manager/export_qualified_key.cpp | Exposes qualified keys to Python. |
| cxx/src/pluginplay/module_manager/export_module_map.cpp | Exposes additional map mutations. |
| cxx/src/pluginplay/module_manager/export_module_manager.hpp | Registers qualified-key bindings. |
| cxx/src/pluginplay/module_manager/export_module_manager_class.cpp | Exposes plugin-aware manager APIs. |
| cxx/src/pluginplay/module_manager/detail_/module_manager_pimpl.ipp | Implements plugin storage, lookup, and defaults. |
| cxx/src/pluginplay/module_manager/detail_/module_manager_pimpl.hpp | Declares plugin-aware internal state. |
| cxx/include/pluginplay/printing/mermaid.hpp | Accepts qualified graph roots. |
| cxx/include/pluginplay/printing/document_modules.hpp | Documents plugin output directories. |
| cxx/include/pluginplay/plugin/plugin.hpp | Defines class-based plugin macros. |
| cxx/include/pluginplay/plugin/plugin_class.hpp | Enables manager access and case-insensitive defaults. |
| cxx/include/pluginplay/module/module_class.hpp | Grants manager attachment access. |
| cxx/include/pluginplay/module_manager/qualified_key.hpp | Defines qualified module identities. |
| cxx/include/pluginplay/module_manager/module_map.hpp | Adds map mutation APIs. |
| cxx/include/pluginplay/module_manager/module_manager.hpp | Includes qualified-key support. |
| cxx/include/pluginplay/module_manager/module_manager_class.hpp | Publishes plugin-aware manager APIs. |
| CMakeLists.txt | Builds the migrated example source. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+101
to
+102
| for(auto& [key, mod] : plugin->m_modules_) | ||
| attach_(qualified_key(name, key).to_string(), *mod); |
jwaldrop107
previously approved these changes
Oct 15, 2026
jwaldrop107
approved these changes
Oct 15, 2026
Contributor
|
🚀 [bumpr] Bumped! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR implements the design from #395. Merging it will break downstream plugins until they swap over. I am in the process of preparing those PRs.