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

Namespace plugin implementation #2644

Merged
merged 5 commits into from Dec 1, 2019

Conversation

@avaris
Copy link
Member

avaris commented Oct 28, 2019

Fixes #2641

  • Creates pelican.plugins as a namespace
  • Moves plugin related code under pelican.plugins
    • pelican.plugins.signals is now the location for signals, pelican.signals is kept
      for backwards compatibility (and could be deprecated in the future)
    • pelican.plugins._utils contains necessary bits for plugin discovery and loading.
      Logic from Pelican class is moved here. Pelican class now just asks for plugins
      and registers them
  • Adds a new command to list installed namespace plugins: pelican-plugins
  • Contains tests for old and new plugin loading

Behavior should be mostly backwards compatible.

  • If you have PLUGINS setting, things should work as before.
  • If you don't have PLUGINS setting and installed namespace plugins, they will be automatically registered.

TO-DO:

  • Documentation update for new plugin format and relevent changes. Something related to this maybe: getpelican/cookiecutter-pelican-plugin#4
  • Remove python2 support. Since native namespace is not supported in python2, tests are failing and will fail.
@justinmayer

This comment has been minimized.

Copy link
Member

justinmayer commented Oct 29, 2019

I tested this implementation with both old- and new-style plugins, and everything described above behaves precisely as expected.

  • Pip-installed namespace plugins register without any plugin-related settings present
  • Old-style plugins function when explicitly enabled via PLUGINS setting
  • Both old & new plugins can be used concurrently if all are specified via PLUGINS setting
  • pelican-plugins lists all installed namespace plugins

I'm working on the documentation, in conjunction with the aforementioned migration from individual plugin CONTRIBUTING files to a revamped Contributing section of the Pelican docs.

Regarding Python 2, it is indeed time to remove support for it in Pelican, and I have taken the liberty of creating an issue to track removing Python 2.7 support. I don't have a strong preference whether that occurs in this PR or a separate one.

Any comments on this pull request from @getpelican/reviewers?

Excellent work on this endeavor, Deniz. Bravo! 👏

@oulenz

This comment has been minimized.

Copy link
Contributor

oulenz commented Oct 30, 2019

Impressive work!

Could you explain what still relies on pelican.signals, i.e. why we can't delete it right away? (I admit I still haven't grasped the mechanism behind signals, so the answer may be obvious...)

(I won't be able to test this until next week.)

@avaris

This comment has been minimized.

Copy link
Member Author

avaris commented Oct 30, 2019

Could you explain what still relies on pelican.signals, i.e. why we can't delete it right away?

Pretty much every single plugin :)... I moved the signals under pelican.plugins because that seemed like the logical place for them. But that also means we need to keep pelican.signals until plugins adjust their imports. I don't have a strong opinion about moving signals (it can stay as is), but since we are going for a restructure in plugins this sounded like a good opportunity to slight logical reorganization (to me at least) in the core too.

PS: tl;dr, on signals: signals is how pelican communicates with plugins. plugins register callback functions to certain "signals", then pelican emits/sends signals at certain stages of the run and callbacks from plugins will do as they please.

@oulenz

This comment has been minimized.

Copy link
Contributor

oulenz commented Oct 31, 2019

So then it's probably desirable to start adjusting the imports in pelican itself? I.e. from pelican import signals -> from pelican.plugins import signals in pelican/__init__.py and any other places?

Also, my understanding of import resolution is limited, but given that signals is defined in pelican/__init__.py, shouldn't from pelican import signals keep working in plugins even if we deleted pelican/signals.py? 🤔

@avaris

This comment has been minimized.

Copy link
Member Author

avaris commented Oct 31, 2019

Oh... I totally missed the import in pelican/__init__.py. Thanks for pointing that out. I'll fix that. I think with that changed, plugins can stay as is.

This should definitely work:

from pelican import signals

I am not 100% sure but this would also work I believe. This does not work. But I did a quick search in the pelican-plugins repo and did not find any plugin using this way of import style:

from pelican.signals import some_signal
@avaris avaris mentioned this pull request Nov 5, 2019
7 of 7 tasks complete
@oulenz

This comment has been minimized.

Copy link
Contributor

oulenz commented Nov 10, 2019

I've also been able to test this now, and new-style and old-style plugins, as well as a mixture of both all work for me!

@justinmayer

This comment has been minimized.

Copy link
Member

justinmayer commented Nov 26, 2019

@avaris: Now that Python 2 support has been removed, would you mind resolving the merge conflicts in this PR and rebasing on current master? Looking forward to getting this merged. 😁

@avaris

This comment has been minimized.

Copy link
Member Author

avaris commented Nov 27, 2019

Yes, I was expecting conflicts and waiting for the merge :).

avaris added 4 commits Oct 27, 2019
* Creates pelican.plugins
* Moves plugin related code under pelican.plugins
  * pelican.plugins.signals is now the location for signals, pelican.signals is kept
    for backwards compatibility
  * pelican.plugins._utils contains necessary bits for plugin discovery and loading.
    Logic from Pelican class is moved here. Pelican class now just asks for plugins
    and registers them
* Contains tests for old and new plugin loading
@avaris avaris force-pushed the avaris:namespace branch from c8bb020 to 87a5c82 Dec 1, 2019
@avaris avaris changed the title WIP: Namespace plugin implementation Namespace plugin implementation Dec 1, 2019
@avaris

This comment has been minimized.

Copy link
Member Author

avaris commented Dec 1, 2019

rebase, cleanup and documentation update: done

Copy link
Member

justinmayer left a comment

Looks great! I made a few minor documentation suggestions.

docs/plugins.rst Outdated Show resolved Hide resolved
docs/plugins.rst Outdated Show resolved Hide resolved
docs/plugins.rst Outdated Show resolved Hide resolved
docs/plugins.rst Outdated Show resolved Hide resolved
docs/plugins.rst Outdated Show resolved Hide resolved
docs/plugins.rst Show resolved Hide resolved
@avaris avaris force-pushed the avaris:namespace branch from 7893763 to 8a56e1f Dec 1, 2019
Copy link
Member

justinmayer left a comment

Fantastic work, Deniz. Bravo!

@justinmayer justinmayer merged commit 48f6275 into getpelican:master Dec 1, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.