Implement Extensions about amulet-editor HOT 13 CLOSED

Here is my prototype of an API.
https://gist.github.com/gentlegiantJGC/835f1d3ffb27dad0c21f8792f8469d07

I have some more ideas to make it better.

from amulet-editor.

There are two parts to dependency management.

The first part is making sure that a plugin's dependencies are available. We have two options:

1. We resolve and install the dependencies when a plugin is installed - pip style
2. We leave it up to the user to install the required dependencies - this is the simpler solution for us and least likely to break

The second part is the activation order.

1. Plugins define dependencies pre-init and we only activate the plugin when all of its dependencies are activated. - see previous API example. This solution will not work with circular dependencies due to all dependencies needing activation first.
2. Plugin on_load can register callbacks for activation of other plugins. This allows all plugins to be activated sequentially and each plugin

I have thought more about the last option and I think it could work but it would be very complex.

I think we should just accept that plugins with circular dependencies are not possible.

from amulet-editor.

Another thing to think about is how we would do static typing for any of this since we cannot directly import the other plugins.

Perhaps first party plugins should be in a module with a fixed path so we can use typing.TYPE_CHECKING to import for typing but have the namespace unavailable at runtime.

from amulet-editor.

Here's some feedback and improvements for the current prototype code:

1. I think it would be better to keep all of the plugins in one dictionary/list and just track whether the plug-in is enabled/disabled/inactive alongside the plugin than keeping them in different lists

2. Should we make views tied to a specific plugin? As of right now a view is entirely separate from the plugin it's supposed to be a view for and the only thing tying a view to a plugin is that the plugin registers the view. This means that if a plugin is disabled then it won't be trivial to determine which views to remove/disable. I think we should make views more linked with the plugins that create them

3. I do really like how accessing another plugins API is through a method call and not an import, we'll be less prone to import errors this way. Should we include an optional version specification so a plugin can error/notify a user if plugin #1 has been updated and plugin #2 utilized an old api that has been removed and thus now incompatible?

4. We could add a decorator to auto register plugins with the system and could potentially move the PluginIdentifier to there instead of needing it to be a property of the class (I'll post another comment with that code snippet once I'm back at my development environment later today)

There's probably a few other feedback or improvement items that I've forgotten or skipped but overall I really like this new system and I think it will be a big positive improvement for both extension developers and for us from a maintainability perspective

from amulet-editor.

1. I agree. I don't want to keep that information in the plugin class itself because it shouldn't be something the plugin can interact with. Perhaps just a dataclass that stores the plugin instance and some other data like that.
2. I agree. Obsidian has all the register methods in the plugin class itself which I assume handles unregistering it when the plugin is disabled. That is a good solution for them but I don't know how we would do that if plugins can define their own API.
3. Yes this is a good idea.
4. I think the better way of defining metadata is with a json file next to the executable code. That way the code would not need to be executed until the plugin is actually activated.

Another thing we need to think about is how a plugin can import code from itself. I think the modules will need to be added to sys.modules

from amulet-editor.

Another thing to think about is how we would do static typing for any of this since we cannot directly import the other plugins.

Perhaps first party plugins should be in a module with a fixed path so we can use typing.TYPE_CHECKING to import for typing but have the namespace unavailable at runtime.

I think that a good way to handle this would be to use Protocol classes. That way we can get static type checking without interfering with any runtime behaviors.

from amulet-editor.

1. I think it would be better to keep all of the plugins in one dictionary/list and just track whether the plug-in is enabled/disabled/inactive alongside the plugin than keeping them in different lists

I have no issue with this, it makes sense and I don't really see a downside.

2. Should we make views tied to a specific plugin? As of right now a view is entirely separate from the plugin it's supposed to be a view for and the only thing tying a view to a plugin is that the plugin registers the view. This means that if a plugin is disabled then it won't be trivial to determine which views to remove/disable. I think we should make views more linked with the plugins that create them

Since the only way for a view to get added to the application is for the application to specifically request it from a plugin, I think that it should be simple enough to register which plugin each view came from when a view is first loaded.

3. I do really like how accessing another plugins API is through a method call and not an import, we'll be less prone to import errors this way. Should we include an optional version specification so a plugin can error/notify a user if plugin #1 has been updated and plugin #2 utilized an old api that has been removed and thus now incompatible?

Agreed, methods over imports sounds preferable.
Version specification should probably be required and not optional since it's simple enough to add and will almost certainly help out in the long run with dependency management.

4. We could add a decorator to auto register plugins with the system and could potentially move the PluginIdentifier to there instead of needing it to be a property of the class (I'll post another comment with that code snippet once I'm back at my development environment later today)

@gentlegiantJGC's idea of storing metadata in a json file sounds good to me. I feel like it being separate could also help us deal with API breaking changes in the future since json parsing is independent of our plugin implementation.

from amulet-editor.

Protocol classes

I was not aware of these. While they look useful I think it would be better to generate stub files which will achieve the same behaviour with no work from us.

from amulet-editor.

I am looking for the best way to continue developing them.
I have looked into gist.github.com which supports a while repository but I can't get it to work.
The alternatives are a new repository just for this like we are doing with the chunk storage prototyping or as a new branch of this repository.
I think I will do the latter for now and we can switch later if we want.

from amulet-editor.

The new version is in this branch.
https://github.com/Amulet-Team/Amulet-Editor/tree/proto-plugin/plugin_prototype

I have made most of the changes that were suggested.
Still need to:

• implement versioning
• implement loading plugin code (I have not decided on the structure)
• Add some example plugins
• Add a mechanic to automatically unload behavour (eg views)

From testing we can load code from an arbitrary path however to support packages we will need to put the package in sys.modules
Relative imports will work with any arbitrary string as the name but to support absolute imports it needs to be the name the package expects.

from amulet-editor.

I have done a little more work on the plugin API.
I have worked out a kind of janky solution that allows plugins to work with absolute importing while also stopping plugins from directly importing other plugins.
It replaces sys.modules with a custom dictionary implementation which modifies the get methods. If a plugin module is requested from a path outside of that plugin it will raise an error. Imports of modules outside the plugin directories and within a plugin work like normal.

The solution is a little janky but I don't see a better way to do it. The alternative is to only support relative imports in plugins which I don't particularly like.

from amulet-editor.

I have just added support for plugin versioning.

from amulet-editor.

I think we can close this now

from amulet-editor.