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

Clarify MVC/Command and multiple matching using regex with MVC #165

Open
Startouf opened this issue Oct 14, 2017 · 9 comments
Open

Clarify MVC/Command and multiple matching using regex with MVC #165

Startouf opened this issue Oct 14, 2017 · 9 comments
Labels

Comments

@Startouf
Copy link
Contributor

Startouf commented Oct 14, 2017

Hello I am a bit confused regarding your MVC + Command definition.

I haven seen a forum/irc so I hope it's Ok to discuss in an issue.

Maybe I am mistaken, but when comparing my experience with rails and other MVC frameworks, I had the understanding of slack-ruby-bot as a Command-Controller-Service-Model-View approach. I'd like to clear up my understanding

To begin with I don't understand how can Commands be separated into Controller-Model-View while preserving custom regex matches. Suppose I want to use custom regex matching on the user input and route several regex to the same action, how would I do that with MVC ? it is not clear if alternate_names accepts something else than string. For example, what if I wanted both calland 呼び出し to route to my #call action ?

I am actually missing some sort of Routes component/naming in the design, ie. a place/file where the expression matching is performed the user sentence to determine the controller/action to be used. I was hoping to be able to register several match() regex and they would all be redirected to the same controller#action. Actually at some point I thought this was what you actually called Commands, and I would rather have seen a command implement something like this

class WhoisCommand < SlackRubyBot::Commands::Base
  # Main command name
  command_canonical_name('whois') 

  # Alternate ways of calling this command
  match(/who is (?<expression> \?.*$/)/) # Also catch "Who is Armstrong ?"
  match(/what can you tell me about(?<expression> \?.*$/)/)

  # Link command to a controller
  def self.call(client, data, match)
    @controller = UsersController.new(client, data, match).show
  end
end

following the Rails way I'd have even imagined something like

command 'whois', to: "users#show"'
match /who is .* \?.*/, to: "users#show"'

Regarding controllers, I have not much to say the name fits well. Maybe I'd say that together Routing + Controller would form the NLP (Natural Language Processing) component for the slack-ruby-bot" responsible for understanding user input

But then, I don't understand what's the Model for you. For people coming from MVC frameworks Model usually refers to objects resolved in the controllers that are then passed to the view, eventually with an intermediate Service steps that helps collecting data needed to build those Models. I would have rather written my controller like this

class UsersController < SlackRubyBot::MVC::Controller::Base
def show
  find_user(match[:expression])
  if @user
    view.show_user(@user)
  elsif @users
     view.desambiguate(@users)
  end
    view.show_404(match[:expression]
  end

  def find_user(expression)
    # scan expression for name, etc.
    response = UserService.find_users_in(match[:expression]) # response.results are User models
    if response.results.size == 1
     @user =  response.results.first
    elsif response.results.size > 1 
      @users = response.results
    end
end

With a different concrete example, assume my chatbot is responsible for finding information about people. Assume a user asks

@bot Who is Armstrong ? Give me his birth date

I'm expecting

  1. Command/routing --> SlackRubyBot::Commands::Base recognizes whois command as it matches /who is (.*) \?.*, and should route to... UsersController#show
  2. controller --> #show should sanitize/check user input (eg calling downcase, etc.) and eventually retrieve special options (like scanning for "give me his birth date"). Based on that the controller calls services like MyOwnInformationDatabaseService, WikipediaService, etc. that are responsible for gathering required material to answer a user query. those info are used to build User models that structure the data
  3. services --> They are actually similar to your SlackRubyBot::MVC::Model::Base but I usually talk about service since this is where the business logic takes place.
  4. models --> They just structure the data, and generate Objects with nice accessors so we can call @user.name @user.linkedin_url
  5. view --> the actual response to the user

What do you think about this ?

I'm actually trying to implement an information chatbot using your MVC design, but at some points names feel wrong, and I'm stuck since I don't understand how to have a MVC with additional match (is it only possible to use regex as alternate names ? how are they transformed ?)

Otherwise your bot engine feels great and I hope I can build some nice bots and contribute to your repo :-)

@dblock
Copy link
Collaborator

dblock commented Oct 15, 2017

@chuckremes care to pitch in?

@m-kind
Copy link

m-kind commented Oct 22, 2017

@Startouf: Basically I ran into the same problem when employing the MVC feature for a small test project. Regarding the list of what you are expecting:

  1. Routing: Maybe some simple glue code already helps. See below in my code example. It appears quite "railsy".
  2. Controller: Implement your own base controller and derive your controller classes from that one. I think it does not require much more than access to client, data and match and you can extend it with helper methods as you like (e.g. a sender method to return sender info).
  3. Services: can (a should) be POROs completely independent of SlackRubyBot.
  4. Models: are also completely independent of SlackRubyBot. Use whatever DB, ORM, KV store you like
  5. Views: I haven't implemented a base view class but it should be quite similar to the controller. I think a view only needs access to the client to say something, upload files etc.

This is my dirty POC code:

# Some glue code...

class SlackBot < SlackRubyBot::Bot
end

class SlackController
  attr_accessor :client, :data, :match

  def initialize(client, data, match)
    self.client = client
    self.data = data
    self.match = match
  end

end

class SlackRouter

  def self.draw(&block)
    instance_eval(&block)
  end

  def self.route(path, **opts)
    controller_name, action = opts[:to].split('#')
    controller_name += '_controller'
    case path
      when Regexp
        SlackBot.match path do |client, data, match|
          controller_name.camelize.constantize.new(client, data, match).send(action.to_sym)
        end
      when String
        SlackBot.command path do |client, data, match|
          controller_name.camelize.constantize.new(client, data, match).send(action.to_sym)
        end
    end
  end

end

# Custom code goes here:

class ApplicationController < SlackController
  def help
    # call a service here...
    #
    # ...and turn this into a call to a view class
    client.say(channel: data.channel, text: 'I need somebody')
  end
end

class WeatherController < SlackController
  def how
    client.say(channel: data.channel, text: 'I am fine')
  end
end

SlackRouter.draw do
  route /\Ahelp\s+me/, to: 'application#help'
  route 'how',         to: 'weather#how'
end

SlackBot.run

Would this come close to what you expect an MVC approach to look like?

@dblock
Copy link
Collaborator

dblock commented Oct 22, 2017

Everyone should be making PRs!

@chuckremes
Copy link
Collaborator

Missed the ping a week ago (on vacation!). Let me look at this in the morning and chime in.

@chuckremes
Copy link
Collaborator

Here are my thoughts on a couple of different topics. In no particular order...

Model

@m-kind and @Startouf are correct that Model isn't really necessary. It was added as a convenience so that the magic variables of client, data, and match can be updated by the controller. I suppose I could have provided this as a mixin module for your own plain-old-ruby-objects but I chose to make it a separate class.

If @Startouf wants to refer to his Models as Service objects, I have no objection. For other programmers who are coming to this with a background in MVC, I think renaming these common elements is a mistake since it undermines their previous knowledge.

MVC Name

@Startouf references something he calls Command-Controller-Service-Model-View. If you want to further subdivide each component with helper classes, I don't have any objections but this is still the MVC pattern.

What I implemented here is model-view-controller pattern similar to Rails (but not identical). Rails is a huge beast of a system and I had no need for all of its functionality for my use-case. So I implemented the bare minimum aspects and borrowed a little bit from Rails. I am not a big fan of all the meta-programming magic that Rails uses so I didn't copy any of it.

The point of the MVC exercise was to allow for bot commands to be testable. Whenever I create a command with any complexity beyond client.say(...) I find that using the MVC approach is superior. It isn't perfect so ideas on how to improve it would be great.

Regex Matching

The original use-case that inspired the creation of the MVC code did not include this feature. However, I think that would be a fantastic feature to add. I'm definitely in favor of this. It would make MVC bots just as powerful as their regular Command bot brothers.

Confusion on how to apply MVC for a bot

I included a full example of a bot using the MVC classes. Look at the inventory example in the examples directory. If there are aspects of the framework that are still confusing, let's focus on those specific problems and try to improve documentation (or method naming or class names, etc).

I'll watch this thread for additional replies.

@Startouf
Copy link
Contributor Author

@chuckremes
Thanks for the hindsights.
Regarding Regex matching, do you have maybe some tips on how I could implement this ? I was thinking that inserting a preprocessing step that would map some regex to some commands understandable by the slack-ruby-bot might be the way to go. I wasn't thinking of plugging a real NLP engine, but some real fast regexes to make the chatbot a bit more friendly

@m-kind
Copy link

m-kind commented Oct 25, 2017

@Startouf: Well, basically you could go for a solution close to what I've suggested in my code snippet above. The simplest solution I could think of is in this gist. Untested and this does lack a view component; but no magic included...

@chuckremes
Copy link
Collaborator

chuckremes commented Oct 26, 2017

I'm going to think out loud here for a minute. Follow along!

The way the controller works now is that it creates a command for each method defined in its class. This assumes a 1-to-1 relationship between the command string and the method name. See code here.

When a bot receives a message, the regular route lookup logic is invoked (literally in Commands::Base#invoke). The command string is converted into a method name according to certain rules (spaces are replaced with underscores, caps are downcased, etc). We then send that method name to the controller instance along with some arguments.

Our dilemma is now to figure out how to support match and scan. Under the covers, command registers a route using match but it expects a perfect match. To support a Regex (e.g. match /^How is the weather in (?<location>\w*)\?$/ from the README) we no longer have an exact command string that we can convert into a method name and send to the controller. We need a way to support N-to-1 mapping (where N is some finite number of matches). The same is true for scan where we need to map N matches to 1 method call.

Since we 1-to-1 map methods defined with def <method_name> in the controller to a bot command, we need some way to tell the controller to register a method using match or scan instead of command so we can get this N-to-1 behavior. I see a few approaches.

  1. Instead of iterating over the methods defined in the controller class and automatically creating commands from each of them, make it explicit. The controller class would need new class methods similar to alternate_name to explicitly register a method for a) an exact match (command), b) multiple match (match), and c) multiple match via scan (scan).
  2. Agree on a method naming scheme for mapping methods such as scan_some_method, match_some_method, and command_some_method. Modify the Controller#register_controller method to detect this naming scheme and create routes to that method using command, match, or scan.

I'm sure there are other ways to skin this cat (see @m-kind's ideas for example... my first suggestion is very similar to his).

I kind of like the first approach. It reduces some of the magic but makes class definitions more verbose. Here's how it might look in practice.

class MyController < SlackRubyBot::MVC::Controller::Base
  def sayhello
    client.say(channel: data.channel, text: "Received command #{match[:command]} with args #{match[:expression]}")
  end
  command :say_hello
  alternate_name :sayhello, :alternate_way_to_call_hello

  def saygoodbye
    client.say(...)
  end
  match :saygoodbye, /^Say goodbye to (?<person>\w*)\?$/

  def lookup_quote
    ...
  end
  scan :lookup_quote, /([A-Z]{2,5})/
end
MyController.new(MyModel.new, MyView.new)

Thoughts?

@chuckremes
Copy link
Collaborator

Oh, and we need to handle operator too. I haven't used that one so it's easy for it to slip through the cracks.

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

No branches or pull requests

4 participants