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

SEP1 - New plugin interface #138

Closed
devoxel opened this issue Dec 7, 2017 · 15 comments · Fixed by #215
Closed

SEP1 - New plugin interface #138

devoxel opened this issue Dec 7, 2017 · 15 comments · Fixed by #215

Comments

@devoxel
Copy link
Collaborator

devoxel commented Dec 7, 2017

As detailed by @sentriz before, we want to modify the core interface to allow people to define commands with decorators.

This is moving towards the goal of chaining multiple steely commands, eg .slag | .snoop to get a snoop slag, without intermediate messages.

We should write out a few proposals on how we could do this. Personally I think abstracting fbchat away is the way to go.

@iandioch
Copy link
Collaborator

iandioch commented Dec 7, 2017

I don't see how defining commands with decorators leads towards that goal, but I approve of both it and the goal regardless.

Agree that abstracting fbchat is a good move, in the meantime we can just write a wrapper for it.

@byxor
Copy link
Collaborator

byxor commented Dec 7, 2017

I don't understand how fbchat code is related to chaining together plugin calls, but I'm all for this.

Edit: Ah, yeah that was dumb. All plugins tightly depend on fbchat.

@devoxel
Copy link
Collaborator Author

devoxel commented Dec 7, 2017

Yeah, I think maybe I did mix whys, but for chaining to work we do need to add our own wrapper, and to do that without dirty hacks we kind of need to make plugins register themselves to steely

@byxor
Copy link
Collaborator

byxor commented Dec 7, 2017

Ideas

❌ Make all plugins return a single message.

This won't work for a large number of reasons.

  • Can't send images.
  • Can't react to messages.
  • Can't send multiple messages.
    etc.

✅ Hide the fbchat bot behind a Bot interface.

(Then simply inject the bot into the plugin)
This requires that all plugins essentially become non-reliant on the idea that the plugins are being sent to facebook.

  • All of our plugins should (theoritically) be compatible with irc, MSN messenger, AIM... you name it.
  • The plugins will require some adjustment, but it's worth doing.
  • This way, instead of injecting a real bot, we can inject a StorageBot that simply records what it was sent. I think this is what Aaron was suggesting.
  • This way, some sort of high level "steelylang processor" can check for pipes and then pass recorded state along to the next plugin accordingly (probably in the message).

Potential interface methods:

  • send_message(message)
  • send_image(image)
  • read_message(how_far_back)

// Reacting to messages is kinda difficult to do generically since facebook reacts (angery etc) are niche and don't line up with the reactions you can do on slack.

@iandioch
Copy link
Collaborator

iandioch commented Dec 8, 2017

@byxor (+ @benmcmahon100 ftb)

idk where your "Make all plugins return a single message." idea came from, or how it's related, but regardless, the Bot interface seems to me to be the best direction.

Your first point, about plugins being compatible with all of those services, is misguided imo. I think a better phrasing would be "partially compatible", with possible extra functionality on services that allow it. Eg. allow steely to try to add a reaction to a message, and just fail gracefully if that operation isn't supported on that platform. This relates to your note at the end.

Sidenote: Images aren't supported by IRC. I guess conforming to IRC would be our first desired platform after Messenger. Worth keeping in mind that this mismatch of message platform features will be a blocker from the outset. I think it's ok for certain commands to only work on certain platforms though.

If we do the above, I think there's not really any adjustment required for plugins (your second point mentions this), if in the interim we create a Bot interface that mostly mirrors the fbchat API. Later on we can worry about refactors and moving away from that.

Not sure what the StorageBot is about. Could you elaborate?

@byxor
Copy link
Collaborator

byxor commented Dec 8, 2017

  • The "make all plugins return a single message" idea is me showing my train of thought to remove fbchat code from the plugins. I'm intentionally showing people that this is a bad idea so nobody tries to do it and we can rule it out completely.

  • Sure, I can get behind the "partially compatible" idea. It just means the Bot interface will be larger and be slightly more prone to leakage.

  • The StorageBot idea is strange. It's simply showing how we can take advantage of the new Bot interface to allow us to pipe plugins together... e.g. .slag Brandon | .shout.

Because a lot of the plugins call sendMessage on the bot they're given, the "results" of the plugin are getting sent to facebook. If we want to pipe commands together, we don't want to send the messages for each plugin to facebook. To solve this, we can inject a different implementation of Bot that records the messages that were sent (instead of sending them) (we can call it StorageBot or something), and then access them later to use as inputs that we're "piping" into the next plugin,

For example...

.slag Brandon | .shout

The slag plugin will be called with a StorageBot (we don't want to send the output, we just want to store it so we can pipe it afterwards).
The shout plugin will be called using output we've collected from the storage bot.

@devoxel
Copy link
Collaborator Author

devoxel commented Dec 8, 2017

I see two ways to go about implementing the new interface: decorators or an overridden class.

Decorators:

@steely.command("translate", input_type=steely.previous)
def main(arguments, prev_message):
    return translate(arguments, prev_message)

@steely.command("flip", output_type=[steely.image, steely.msg])
def main(args):
    result = flip()
    return images[result], make_response(result)

@steely.command("buy", parent="linden")
def main(args):
    return buy_lindens(args)

Decorators was the original proposal and makes steely more involved in how commands operate.

Pros:

  • Provides steely with all the information about various inputs and outputs on a function.

  • Removes the current hack that involves dynamically importing all subfiles. This way things like
    non flat directory structure are easily implemented.

  • A pretty looking api (with refinment to the arguments) that makes it fairly easily to add commands.

  • Avoid race conditions (to some degree) with previous messages, since they are requested immediatly.

Cons:

  • We have to import a steely library, adding a weird kind of a dependency graph. That being said it could
    be handled in some kind of steelylib library

  • Heavily constrained input/output method.

  • Lots of work to implement (both on the steely side and the conversion side)

Same structure, new API

def tranlate(bot, args):
    bot.send(do_tranlation(args, bot.prev_message()))
    return

def flip_coin(bot, args):
    result = flip()
    bot.send_image(images{result])
    bot.send(make_response(result))
    return

def main(bot, args):
    if bad_arguments(args):
         return
    bot.send(sub_command(args[0], args))
    return

Pros:

  • No inherent dependency reliance on steely
  • Gives the plugins some level of autonomy -- they can send what they please.
  • Allows plugins to handle failures properly, as we could raise exceptions (or return errors on send functions) and these could be handled nicely. I'm not sure if this is a real use case though.
  • Easy to integrate.

Cons:

  • Steely has to handle cases it doesn't like (like sending an image through a pipe)
  • Results in repeated code though, like subcommand structures. This isn't neccessarly a bad thing though, and we could perhaps
    create a steelyhelper library to cover things people might want
  • Extending various pieces is a bit harder

@devoxel
Copy link
Collaborator Author

devoxel commented Dec 8, 2017

ping @sentriz

@byxor
Copy link
Collaborator

byxor commented Dec 8, 2017

Sorry for responding so quickly without giving everything a read, but... RE:

@steely.command("translate", input_type=steely.previous)
def main(arguments, prev_message):

Why should a plugin know where its input is coming from? That means you need to write the plugin multiple times to accommodate input from different places.


I like the idea of subcommands.


RE:

The Bot interface results in repeated code though, like subcommand structures. This isn't neccessarly a bad thing though, and we could perhaps create a steelyhelper library to cover things people might want.

Subcommands are a separate concern. The bot shouldn't have any idea about subcommands; a generic subcommand library should be a dependency of the plugin rather than something tightly coupled to the bot.

@devoxel
Copy link
Collaborator Author

devoxel commented Dec 8, 2017

The code examples are very loose and just there to give you an idea, but essentially that just says pass in some input that isn't in the arguments, and could be generalized to that. Steely could pipe in the input from another source, etc.

@byxor
Copy link
Collaborator

byxor commented Dec 8, 2017

Why not just take the input as a parameter? That way we don't need to know where it's coming from, but some higher level piece of code needs to be responsible for passing the correct input in.

Where a function gets its input is not the responsibility of that function itself.

@devoxel
Copy link
Collaborator Author

devoxel commented Dec 8, 2017

Yeah I mean, that's the general idea. Having it as an input works grand, but with an argument we basically are saying to steely we don't ever take any auxilary input (apart from arguments). This means essentially that it doesn't read from stdin.

Like I said though, the exact api for the two code samples isn't super important, and the details can be ironed out once we a general consensus.

@byxor
Copy link
Collaborator

byxor commented Dec 8, 2017

Vague idea/spitballing:

Instead of saying this function doesn't read from the chat, we could mark functions as i can read from the chat using a decorator. That very decorator could give the plugin function access to certain chat-reading functionality.

That way it's impossible for a plugin to say i dont read from the chat and then do it.

@CianLR
Copy link
Member

CianLR commented Dec 8, 2017

I think Aaron's proposing the input_type thing so we know what shape the function inputs take (is it expecting one, two, none?). As opposed to actually rigidly defining where the input comes from

@iandioch
Copy link
Collaborator

We can use Dipla's value routing features to implement command chaining.

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