publiclab / plotsbot

A bot for Public Lab
GNU General Public License v3.0
17 stars 28 forks source link

Modularity of behaviors? #39

Closed jywarren closed 7 years ago

jywarren commented 7 years ago

Hi, @ryzokuken - so are you calling things like greet and help "services" then, and not "behaviors"?

In any case, I see the help behavior is inside util.js and I wonder if we could make each Behavior its own submodule, included via a require() and with its own internal/external API. There could be a folder at /src/behavoirs/____ which includes each one. Then we could just have a listing of them required in, here:

https://github.com/publiclab/plotsbot/blob/master/utils.js#L32

jywarren commented 7 years ago

Ah, i think i see the distinction -- tell me if this is right:

  1. services are a "way that the bot acts generally for an interface" -- so a chatbot is a set of ways-to-act relevant to some types of interfaces, like irc.
  2. behaviors are "specific actions a service may perform" -- so, a chatbot might hear "help" and respond with a helpful message.

And services are needed because multiple interfaces may use similar patterns of action, so irc and matrix could both use the chatbot service?

jywarren commented 7 years ago

But for this idea of encapsulating the behaviors each in one file, talk with @ccpandhare -- see how this is being done in the modules over there: https://github.com/publiclab/image-sequencer/tree/master/src/modules

ryzokuken commented 7 years ago

@jywarren you got it. Services are basically groups of behaviors grouped on the basis of function. eg: help behavior comes under the chatbot service and so on. I am willing to discuss this. Hopefully, we'll make the codebase more legible for new contributors. I agree that the utils file makes things extremely confusing.

jywarren commented 7 years ago

That's great -- can we build out a part of the README that says this very specifically? I had been confused about services vs. interfaces or behaviors. Maybe one section near the top that says plotsbot is made of these three components, and interfaces is just the functional connection to different external applications, while services describes actions associated with a "general type" of service, like all chatbots (which could be implemented over different interfaces like IRC or Slack). Behaviors are specific responses that services have to input.

So this issue could be broken into:

jywarren commented 7 years ago

OK, so copying in my comment from #13, this is fleshing out the 2nd checkbox from my last comment:

Let's get the behaviors into their own folder and standardize the way to add a new behavior accordingly, i.e. (and adding this to the README):

  1. add a new file to /behaviors/ following the format in the help behavior
  2. add a new line to /behaviors.js (included into util.js?) requiring your new file
  3. add a test for your new behavior, showing how it's supposed to work, following the example in /spec/help_spec.js (or equivalent)

This should make it really easy to add new behaviors, and to bulk that out before we add a new interface. Make sense?

jywarren commented 7 years ago

And now that I look at it, utils.js will just become behaviors.js once you move all the help behaviors into its own file.

jywarren commented 7 years ago

And then utils.messageResponse(this.nick, parsed); becomes behaviors.run(this.nick, parsed); -- although maybe this.nick needs to be clarified -- botNick so we don't accidentally think it's the real person's nick? Or is it?

https://github.com/publiclab/plotsbot/blob/master/services/chatbot.js#L38

jywarren commented 7 years ago

Ah, oops, it is the real person's nick. Better change that to clarify! Maybe... behaviors.run(theirNick, parsedMessage);?

ryzokuken commented 7 years ago

@jywarren great ideas. Would love to discuss this with you in person if possible and get this sorted. Then I will change the help and greet behaviors to conform to our new standards and add this specification to the README. This would definitely improve the workflow, make contributor's jobs and our lives easier.

jywarren commented 7 years ago

Hi, Ujjwal - I'm sorry but I don't have time today to talk in person, and in general I often don't, as you have probably realized. For this I apologise but there are a lot of projects going on right now.

Do you think you could meet with Ananyo and discuss things, and post questions and clarifications on GitHub so I can provide feedback when I'm able? You two are a great team and your work to date is great. I'm sure you'll be able to figure things out.

On Jun 17, 2017 10:22 AM, "Ujjwal Sharma" notifications@github.com wrote:

@jywarren https://github.com/jywarren great ideas. Would love to discuss this with you in person if possible and get this sorted. Then I will change the help and greet behaviors to conform to our new standards and add this specification to the README. This would definitely improve the workflow, make contributor's jobs and our lives easier.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/publiclab/plotsbot/issues/39#issuecomment-309217997, or mute the thread https://github.com/notifications/unsubscribe-auth/AABfJwil-eKwr3ajweK8wExBMd00uL94ks5sE-EYgaJpZM4N84l5 .

ryzokuken commented 7 years ago

@jywarren okay, I totally get it. Will post the results of our discussions on Github today. I have a few ideas. Hopefully, you will like it.

ananyo2012 commented 7 years ago

Hi @ryzokuken @jywarren I guess a proper tree structure regarding the modules should be thought of so that we are clear with the approach.

ryzokuken commented 7 years ago

@ananyo2012 that's a great idea. We could make a simple diagram to explain it.

jywarren commented 7 years ago

+1!

On Sat, Jun 17, 2017 at 2:39 PM, Ujjwal Sharma notifications@github.com wrote:

@ananyo2012 https://github.com/ananyo2012 that's a great idea. We could make a simple diagram to explain it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/publiclab/plotsbot/issues/39#issuecomment-309232560, or mute the thread https://github.com/notifications/unsubscribe-auth/AABfJ3bjPZlfNUR63BjNfD9pohnZzPiqks5sFB1YgaJpZM4N84l5 .

ryzokuken commented 7 years ago

Our current directory structure

.
├── bot.js
├── config.js
├── config.js.sample
├── interfaces
│   ├── cli.js
│   └── irc.js
├── LICENSE
├── package.json
├── README.md
├── services
│   └── chatbot.js
├── spec
│   ├── chatbot-spec.js
│   └── support
│       └── jasmine.json
├── utils.js
├── yarn-error.log
└── yarn.lock

4 directories, 14 files
ryzokuken commented 7 years ago

A possibility

.
├── beheviors
│   ├── index.js
│   └── modules
│       ├── greet.js
│       └── help.js
├── bot.js
├── config.js
├── config.js.sample
├── interfaces
│   ├── cli.js
│   └── irc.js
├── LICENSE
├── package.json
├── README.md
├── spec
│   ├── chatbot-spec.js
│   └── support
│       └── jasmine.json
├── utils.js
├── yarn-error.log
└── yarn.lock

5 directories, 16 files
ryzokuken commented 7 years ago

A rough specification for behaviors

type: String => what kind of behavior is it. This will allow the bot to determine when to trigger that 
particular behavior. For our case, we have two types of behaviors right now. `join` behaviors eg: greet 
and `message` behaviors eg: help.

identifier: String => only applies for behaviors with type `message`. This identifier tells the bot when to 
trigger that particular behavior (when the identifier is a part of the message). For example, the identifier 
for the `help` behavior is the keyword "help".

action: Function => This is the function that will be called by the bot with appropraite standard 
arguments when the behavior is triggered. These "standard" arguments will be specified shortly.

We could add the class Behavior to a folder called models or inside the behaviors folder itself. We could either write a class MessageBehavior extends Behavior to include the extra property to just include the indetifier property in the original class and set it to undefined whenever unneeded or set it anyway and not use it. (I prefer the last one)

jywarren commented 7 years ago

What about "trigger" instead of type and " keyword" instead of identifier? I don't feel strongly about it but just a suggestion.

On Jun 18, 2017 4:29 AM, "Ujjwal Sharma" notifications@github.com wrote:

A rough specification for behaviors

type: String => what kind of behavior is it. This will allow the bot to determine when to trigger that particular behavior. For our case, we have two types of behaviors right now. join behaviors eg: greet and message behaviors eg: help.

identifier: String => only applies for behaviors with type message. This identifier tells the bot when to trigger that particular behavior (when the identifier is a part of the message). For example, the identifier for the help behavior is the keyword "help".

action: Function => This is the function that will be called by the bot with appropraite standard arguments when the behavior is triggered. These "standard" arguments will be specified shortly.

We could add the class Behavior to a folder called models or inside the behaviors folder itself. We could either write a class MessageBehavior extends Behavior to include the extra property to just include the indetifier property in the original class and set it to undefined whenever unneeded or set it anyway and not use it. (I prefer the last one)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/publiclab/plotsbot/issues/39#issuecomment-309264127, or mute the thread https://github.com/notifications/unsubscribe-auth/AABfJ05ulbc5Fq1f-yidyONqifrW2uH0ks5sFN_ygaJpZM4N84l5 .

ryzokuken commented 7 years ago

@jywarren any word that captures the meaning of the variable best would work for me. What are your thoughts about the plan overall?

jywarren commented 7 years ago

I like it -- did you get rid of /services/ for some reason? I thought that was fine as it was. I also think to avoid too many folders, we could have /behaviors.js and /behaviors/____.js

On Sun, Jun 18, 2017 at 8:37 AM, Ujjwal Sharma notifications@github.com wrote:

@jywarren https://github.com/jywarren any word that captures the meaning of the variable best would work for me. What are your thoughts about the plan overall?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/publiclab/plotsbot/issues/39#issuecomment-309275255, or mute the thread https://github.com/notifications/unsubscribe-auth/AABfJ8t0JqVyd5FiYwdDqcV3N55G0qoWks5sFRofgaJpZM4N84l5 .

ryzokuken commented 7 years ago

@jywarren behaviors.js and behaviors/*.js sounds great too. Let's finalize it then.

As services were nothing but groups of behaviors, I don't think we'd need services anymore.

What about the specification though, does that sound okay to you?

jywarren commented 7 years ago

It sounds great. Let's start a PR, with the documentation, and build it out!

On Sun, Jun 18, 2017 at 12:28 PM, Ujjwal Sharma notifications@github.com wrote:

@jywarren https://github.com/jywarren behaviors.js and behaviors/*.js sounds great too. Let's finalize it then.

As services were nothing but groups of behaviors, I don't think we'd need services anymore.

What about the specification though, does that sound okay to you?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/publiclab/plotsbot/issues/39#issuecomment-309287855, or mute the thread https://github.com/notifications/unsubscribe-auth/AABfJ63B961AUs92VquaMxc4EsbK9P5kks5sFVAvgaJpZM4N84l5 .

ryzokuken commented 7 years ago

The standard arguments to behavior actions

Join behaviors

  1. channel: String => The channel on which the user joined. As DM channels cannot be joined, this would be more useful once the bot is deployed on multiple channels (which it will be soon when it hits v1.0.0)

  2. username: String => The username of the user who just joined.

Message Behavior

  1. from: String => the nick of the user who sent the message.

  2. to: String => the channel in which the message was recieved. If the message was a DM, this would be equal to the bot's nick

  3. message: Array => The different words of the message, stripped of all punctuation. Also, the nick of the bot and the identifier of the behavior are removed from the list of the words.

Note: Notice how the bot's nickname is referred to as nick while other users' nicknames are referred to as username. @jywarren are you satisfied with this terminological distinction?

jywarren commented 7 years ago

I think if it regularly shows up as bot.nick, that's fine, but this.nick is not great. Can you internally alias "this" to "bot" wherever you can? It's good practice anyway not to blindly use "this" since it has different meanings throughout a source code file.

On Jun 18, 2017 12:42 PM, "Ujjwal Sharma" notifications@github.com wrote:

The standard arguments to behavior actions Join behaviors

1.

channel: String => The channel on which the user joined. As DM channels cannot be joined, this would be more useful once the bot is deployed on multiple channels (which it will be soon when it hits v1.0.0) 2.

username: String => The username of the user who just joined.

Message Behavior

1.

from: String => the nick of the user who sent the message. 2.

to: String => the channel in which the message was recieved. If the message was a DM, this would be equal to the bot's nick 3.

message: Array => The different words of the message, stripped of all punctuation. Also, the nick of the bot and the identifier of the behavior are removed from the list of the words.

Note: Notice how the bot's nickname is referred to as nick while other users' nicknames are referred to as username. @jywarren https://github.com/jywarren are you satisfied with this terminological distinction?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/publiclab/plotsbot/issues/39#issuecomment-309288601, or mute the thread https://github.com/notifications/unsubscribe-auth/AABfJ0-F4KRMRLGrGK0JjFpPWHrkwoh7ks5sFVNZgaJpZM4N84l5 .

ryzokuken commented 7 years ago

@jywarren Sure, why not.

ryzokuken commented 7 years ago

@jywarren while I am starting to work on the PR, should we make this a major version change? I mean, there are breaking changes in the application API, right? I am new to semver.

jywarren commented 7 years ago

yep, agreed, api-breaking. Super!

On Sun, Jun 18, 2017 at 1:07 PM, Ujjwal Sharma notifications@github.com wrote:

@jywarren https://github.com/jywarren while I am starting to work on the PR, should we make this a major version change? I mean, there are breaking changes in the application API, right? I am new to semver.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/publiclab/plotsbot/issues/39#issuecomment-309289966, or mute the thread https://github.com/notifications/unsubscribe-auth/AABfJ3qJ0JGlQ44_US0Hfdnk98ul_Kz7ks5sFVlSgaJpZM4N84l5 .