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

[WIP] Add pluggable transport registry #188

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

ad-m
Copy link
Collaborator

@ad-m ad-m commented May 14, 2018

It is possible that built-in transports should also be rewritten in this way.

@ad-m ad-m changed the title Add pluggable transport registry [WIP] Add pluggable transport registry May 14, 2018
@coddingtonbear
Copy link
Owner

This isn't a bad pattern to follow! I had for a while considered refactoring the transports such that we could use setuptools' entry_points for defining new ones (this is how flake8, click, pylons, etc. handle registering plugins) . The upside of that is that packages adding support for new transports could be released and automatically detected without any effort. The downside, though, is that using entry_points requires that people implementing new transports write an installable package with its own setup.py file and associated baggage. Python packaging is a pain in general; so that is kind of a high bar.

Going this route does let people circumvent that whole set of problems, and although there are limitations of this approach (the module registering itself must somehow be imported before it'll appear in the registry) -- there're common patterns used in Django for handling that class of problems already given that use of signals already requires that. So, I think you've made the right choice!

@ad-m
Copy link
Collaborator Author

ad-m commented May 15, 2018

@coddingtonbear , in up-to-date django there is AppConfig.ready which is recommended way to register signals (we miss that in our docs).

@coddingtonbear
Copy link
Owner

Yeah -- it wouldn't at all surprise me if our docs don't mention AppConfig.ready. I'm pretty sure django-mailbox predates that feature entirely! Back when I was writing this, the recommended pattern was still that signals be either imported by models.py or included there directly.

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

Successfully merging this pull request may close these issues.

3 participants