Skip to content

Refactor plugin - #607

Closed
geigerzaehler wants to merge 19 commits into
beetbox:masterfrom
geigerzaehler:plugin-registry
Closed

Refactor plugin#607
geigerzaehler wants to merge 19 commits into
beetbox:masterfrom
geigerzaehler:plugin-registry

Conversation

@geigerzaehler

Copy link
Copy Markdown

In this PR we try to develop a new plugin API. The API has two parts: the BeetsPlugin class to be used by plugins and the functions beets uses to interact with plugins. See also the wiki.

At the moment the only change is the addition of a Registry class to encapsulate all interactions with plugins.

TODO
  • Use BeetsPlugin methods to expose event handlers. If I understood @sampsyo in Exception handling for item writes #603 correctly this means that a plugin simply needs to implement a on_write method to register a 'write' event handler.
  • Think of a clever way to expose the default registry without a calling plugins.registry or migrate all code to the new registry API.
  • Make subclasses of BeetsPlugin singeltons by overwriting the __new__ function. Obsolete
  • Clarify the usage of namespace packages. At the moment I’m leaning towards keeping then, since they actually work quite well if we are careful.
  • Devise a way to migrate to the new API or provide a compatibility layer.

@sampsyo

sampsyo commented Mar 13, 2014

Copy link
Copy Markdown
Member

Cool! Thanks for getting the ball rolling.

FWIW, this bit of refactoring—gathering up the loading logic and some other stuff into a PluginRegistry class—seems reasonable to merge sooner rather than later. It doesn't cause any API breakage—right?

[Side note: I'm going to be slow on responding to this and your other recent changes, @geigerzaehler, for a a few days due to RL obligations. Sorry for my negligence! I'll try to get to everything before long.]

@sampsyo

sampsyo commented Mar 14, 2014

Copy link
Copy Markdown
Member

I wrote a few more design notes on a new wiki page.

@geigerzaehler

Copy link
Copy Markdown
Author

Just came across instructions to extend the MediaFile class through plugins. With the recent rewrite of MediaFile this is completely out of date. @sampsyo, are you aware of plugins that use this? If not we should probably remove this functionality altogether.

@sampsyo

sampsyo commented Mar 23, 2014

Copy link
Copy Markdown
Member

Yes, good point—that was of dubious utility anyway (I can't even recall the original excuse for adding it). Let's dispose of that functionality.

@geigerzaehler

Copy link
Copy Markdown
Author

I added some thoughts on singleton to the wiki.

@sampsyo

sampsyo commented Mar 24, 2014

Copy link
Copy Markdown
Member

That's a good point, @geigerzaehler — it would be easier to manage the instance lifetime externally.

geigerzaehler pushed a commit to geigerzaehler/beets that referenced this pull request Mar 24, 2014
geigerzaehler pushed a commit that referenced this pull request Apr 4, 2014
Backported from #607.
See #607 (comment)

This will be return in #644.
@geigerzaehler

Copy link
Copy Markdown
Author

After giving it some thought, I’ve come to the conclusion that it’s best to move away from the namespace approach to plugins. Instead a plugin should just be a python module name beets_<plugin>. This has several advantages.

  • Eliminates boilerplate code
  • No issues with package managers
  • Locations easily controlled with sys.path
  • Simple and flexible: plugin = module

Of course we should retain the core plugins in beetsplug.<plugin>.

@sampsyo

sampsyo commented Apr 8, 2014

Copy link
Copy Markdown
Member

Yep, seems right to me. I'm still divided on whether beetsplug or something like beets.plugin is the best containing package name for the included plugins. Not that this detail matters very much. 😃

@geigerzaehler

Copy link
Copy Markdown
Author

I just tried to write code for the new naming convention that keeps backwards compatibility with beetsplug: It’s hard 😓! And I don't think it's worth it—we should just rely on the new mechanism. Also in favor of beets.plugin.<core plugin>.

@sampsyo

sampsyo commented Apr 10, 2014

Copy link
Copy Markdown
Member

True; it does seem hard! We might need to make a clean break, which would be fine.

geigerzaehler pushed a commit to geigerzaehler/beets that referenced this pull request Apr 12, 2014
geigerzaehler pushed a commit to geigerzaehler/beets that referenced this pull request Apr 13, 2014
geigerzaehler pushed a commit to geigerzaehler/beets that referenced this pull request Apr 18, 2014
@jtpavlock

Copy link
Copy Markdown
Contributor

This seems like it would be better served as a WIP reference to someone else that would like to continue it on a new PR

@stale

stale Bot commented Nov 18, 2020

Copy link
Copy Markdown

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants