EliAndrewC / sideboard

BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

Plugins should be able to import from other plugins #66

Closed EliAndrewC closed 10 years ago

EliAndrewC commented 10 years ago

As I consider the work I have ahead making the MAGFest code a Sideboard plugin, I'm realizing that I really need to write plugins which import from other plugins.

My use case is this: I have a bunch of code which configures automated emails which I'd like to move to be a separate plugin, since much of it changes every year and within the same year every different event has its own emails, so it'd be nice to have these configured in a different repo. Here are some examples of how these emails are configured:

Reminder(Attendee, 'MAGFest payment received', 'attendee_confirmation.html',
         lambda a: a.paid == HAS_PAID)

MarketplaceReminder('Your MAGFest Dealer registration has been approved', 'dealer_approved.html',
                    lambda g: g.status == APPROVED)

Reminder(Attendee, 'MAGFest Badge Confirmation Reminder', 'confirmation_reminder.txt',
         lambda a: days_after(7, a.registered) and a.placeholder and a.first_name and a.last_name)

Reminder(Attendee, 'Last Chance to Accept Your MAGFest Badge', 'confirmation_reminder.txt',
         lambda a: days_before(7, PLACEHOLDER_DEADLINE) and a.placeholder and a.first_name and a.last_name)

StopsReminder('MAGFest shifts available', 'shifts_created.txt',
              lambda a: state.AFTER_SHIFTS_CREATED and a.takes_shifts)

StopsReminder('Final reminder to meet your MAGFest hotel room requirements', 'hotel_hours.txt',
              lambda a: days_before(7, UBER_TAKEDOWN) and a.hotel_shifts_required and a.weighted_hours < 30)

There's a DaemonTask which every few minutes queries the database and checks whether any new emails need to be sent. After an email is sent we write a record back to the database to make sure we don't send the same email multiple times.

So this is not strictly impossible to get working using only sideboard.lib.services since we could use the Crud API to get everything from the database and define a service to expose all of the configuration. However, that implementation would be significantly more complex than what we have now, for not a lot of benefits. (If someone isn't clear why this is true, I can go into specifics.)

So what I'd like to be able to do is just say from uber.common import * in the email plugin and have that work. Which is actually fine conceptually, but the problem is that this is not currently guaranteed to work since it means that the uber plugin would need to be imported before the uber_emails plugin.

There are a lot of ways we could define this, but for now I'm inclined to go with the simplest, which is to have a Sideboard config options which lets you specify which plugins get imported first. So if I have 4 plugins foo, bar, baz, and baf and then I added a config like like this:

imported_first = ["bar", "baf"]

then bar would be the first plugin imported, followed by baf, followed by each of the remaining plugins in an undefined order.

A cleaner implementation would be to allow plugins to tell Sideboard that they depend on other plugins, like uber_emails would declare somehow that it depends on uber and then Sideboard would figure out a sensible import order from there. However, I can't think of an easy way for this to be specified, since the plugins' config files are parsed as they're imported, so their config files are not a good place for this. I guess we could have a separate config file just for import ordering, but that seems messy.

EliAndrewC commented 10 years ago

@robdennis and I chatting for awhile offline, and he pointed out that cross-virtualenv imports are scary and weird enough that we should make the programmer need to explicitly do something like use a context manager, e.g.

with cross_venv_import:
    from uber.common import *

I think this is a good idea. At present we can use a context manager which literally does nothing, but not mention this in the documentation, and eventually even make the context manager required.