brettstimmerman / jabber-bot

Easily create simple regex powered Jabber bots.
BSD 3-Clause "New" or "Revised" License
91 stars 30 forks source link

Multi param commands #1

Closed vivien closed 13 years ago

vivien commented 13 years ago

Hi Brett,

I've allowed the bot to have multiple parameters for its commands.

This adds 2 things:

The things that change for the user or:

I've updated the readme, the sample and methods doc as well.

Regards, Vivien.

brettstimmerman commented 13 years ago

I think this is fine for the sample bot, but I don't want to lose the examples already in place.

The sample bot is a dead simple example to get you up and running quickly. From there, it's on you to build out your bot in any way you like. I don't want to prescribe any more than necessary to show, at a most basic level, how things work.

vivien commented 13 years ago

I don't really understand what you mean. The only thing existing bots need is to add parenthesis around ".+" in the command regex, a two-chars commit to have multiple parameters. What are your examples already in place? I think it's a shame not to make a nice little project like this evolve, especially when the new feature is quite simple and make it a little bit more flexible. But that's up to you... ;) Regards, v0n.

brettstimmerman commented 13 years ago

It's up to the user to create their own commands. The sample bot is just a very basic example of something that works. It's my hope that users will begin with the sample bot as a starting point, and build their own commands on top of it once they understand how it works.

It's not the project's place to say what commands are useful. It completely depends on what the user wants their bot to do.

vivien commented 13 years ago

Ok Brett, I think you totally misunderstood what I've changed! I've not added new commands, I've just added the ability to have multiple parameters in commands. The rand2 command added in the sample file is just here to show how multiple parameters commands work. It can be removed from the sample file if you'd like to keep this file dead simple for users. I hope I was a little bit more clearer.

brettstimmerman commented 13 years ago

Sorry for the back and forth here. I appreciate the conversation. I think I do understand your change, but I'm afraid you are not understanding why I feel it doesn't belong in jabber-bot.

My point is, jabber-bot only cares that a command has a regex and a callback when the regex is matched. Beyond that, it does not care what the regex is or what code the callback contains.

If a user wants to create a command that accepts multiple parameters, that is fine. It's up to the user to create a regex/callback pair that satisfies their needs. Jabber-bot doesn't attempt to know what you want it to do, or force on you anything it may be capable of doing. It provides you the tools to (hopefully) make it do what you want it to do. It's putty in your hands to mold as you see fit.

With that in mind, your rand2 command would make a good example of a command that accepts one or more params. If you feel like the docs could do a better job of providing richer examples, like the rand2 example, or other potential "tips n' tricks," that is one thing. But I don't think that it's worth adding rand2 at this point.

vivien commented 13 years ago

You said "If a user wants to create a command that accepts multiple parameters, that is fine. It's up to the user to create a regex/callback pair that satisfies their needs.". That's not possible for the moment because your code (in your current master branch) does not allow it. To allow that, the parse_command method needs to use a little trick with match.captures instead of a sub if the message includes a space. And this is just what I've done in my multi-param-commands branch. Just have a quick look at the "Files changed" section of the pull request (note that I've also removed the rand2 example in the sample file so it's still as dead simple as before.) I really hope that now you understand that I've not added a command, but just an ability. Vivien.

brettstimmerman commented 13 years ago

Ahh, ok. Now I understand. I'll take a closer look this week. Thanks for putting up with me being so dense before.

vivien commented 13 years ago

Hi Brett, This pull request was a bit messy and I have to say, I wasn't really confortable with the rebase tool. Now that, I think, you understood the idea of my request, I'll close this one, rebase and clean the code, and send you a fresh one.