joshuaskelly / twitch-observer

Turn Twitch chatter into Python events
MIT License
26 stars 6 forks source link

Consider adding "on_command" decorator #61

Open senpos opened 6 years ago

senpos commented 6 years ago

Since we already have on_event decorator, it could be also nice to have the same thing for chat commands.

Examples

# User can set command prefix during Observer initialization with the "!" as default
observer = twitchobserver.Observer(
    ...,
    command_prefix = '!',
    ...
)

# Default command
# usage: !social
@observer.on_command('social')
def social_handler(event):
    pass

# User can set multiple aliases for the same command
# usage: !song | !песня
@observer.on_command('song', 'песня')
def song_handler(event):
    pass

# User can override a prefix for specific command
# usage: %promo
@observer.on_command('promo', prefix='%')
def promo_handler(event):
    pass

# User can pass arguments to the handler context
# usage: !weather London Berlin
@observer.on_command('weather', pass_args=True)
def weather_handler(event, args):
    print(event.message)  # '!weather London Berlin'
    print(args)  # ['London', 'Berlin']

Another important moment about events handling: order of execution. Do you allow to set multiple handlers for the same event / command? If yes, in what order they will execute? If no, which one will execute?

I was thinking about on_command implementation. I wanted to re-use some of the logic from on_event, but could not come up with solution. It is just easier to create new decorator from scratch. So, I'd like to learn something new from you :)

joshuaskelly commented 6 years ago

I'd like to keep the Observer class a pretty lightweight Twitch Message -> Python Event class.

In practice we always add this functionality so I think it would be good to add this logic in a specialized subclass. I'm thinking something like:

class SimpleChatCommandObserver(TwitchChatObserver):
    def on_command(*args, **kwargs):
        """Implementation details..."""

Then it would be used basically as you proposed (thank you for the examples!).

Thoughts?

senpos commented 6 years ago

On the one hand, it is reasonable and pretty common thing to make another class which will extend functionality of the basic class with specific features (command handling, in this case), I've seen this pattern a lot.

On the other hand, this is pretty core functionality and probably will be used most of the time, it correlates with event handling directly, since it is just kind of cool wrapper around it with few most-used things already implemented. It also can be confusing which class to pick to start doing things, it requires more time to read the docs (which is not bad, but not always necessary for such common things). I believe, user will end up using CommandChatObserver anyway, because it has all the features ChatObserver has and even more.

So, my point here: adding on_command decorator and a single kwarg with default into ChatObserver class won't do anything bad, since it can be easily ignored by a user and he might not even notice the changes, because code won't break.

Both of the ways are acceptable, whatever you choose will be cool. It is just a matter of taste, i think. 😋

joshuaskelly commented 6 years ago

Oh, that was a hasty example. The API will certainly be more user friendly. 😄

PythooonUser commented 6 years ago

Personally I would prefer adding on_command to the Observer class since its just on_event with an additional if statement^^

But I get that @joshuaskelly wants to keep the initial Observer class lightweight. I think there are use cases where people aren't interested in commands and just want to listen for messages or log join/part events. So going with the additional class is maybe the best solution for now. However, it introduces more complexity to the project.

Another thought: What about refactoring decorators (maybe we add even more in the future) to their own namespace?

from twitchobserver.decorators import on_event, on_command

Not sure how to handle the subscription process then tough.

senpos commented 6 years ago

What about refactoring decorators to their own namespace?

Since they rely on Observer's internal stuff, it is not possible or would result ugly API.

I think there are use cases where people aren't interested in commands and just want to listen for messages or log join/part events.

Still, I am not sure why is this a problem. Decorator and optional kwarg don't perform anything themselves, they just live inside of the class. Those people should and would ignore them.

joshuaskelly commented 6 years ago

Commands vs Event Listeners

A command is different from an event listener as commands are more mutable than event listeners. It's a very common use case for a chat moderator to add a new command during a stream.

Aliasing

Also, should aliases have a many to one relationship with command handlers?

Prefixing

Prefixing is useful to correctly identify user issued commands from normal chat. It is nice to allow defining custom prefix to avoid collision with other bots. Do we need a per command custom prefix, or is a global prefix fine?

Proposed Class API

from twitchobserver import Observer

class SimpleChatCommandObserver(Observer):
    def __init__(self, command_prefix='!'):
        """Constructor"""

    def add_command(self, *alias, callback):
        """Add a command handler for the given aliases"""

    def remove_command(self, alias):
        """Remove a command handler for the given alias"""

    def update_command(self, alias):
        """Modify an exisiting command"""

    def on_command(self, *alias):
        """Decorator for command handlers for given alias"""

I'd appreciate feedback.

I'm starting to worry that I'm over-engineering this. ☹️

PythooonUser commented 6 years ago

Also, should aliases have a many to one relationship with command handlers?

Yes

Do we need a per command custom prefix, or is a global prefix fine?

Both. A global one set per default and for each command a possible custom one. Although I wonder if there are use cases for that?

I'm starting to worry that I'm over-engineering this.

Since you said that having commands change at runtime is a common use case I think you are not over-engineering it. How about adding a decorator for the add_command and on_command methods (fusing them into one decorator) in order to have a quick access for users that do not need the fancy features like updating commands during runtime?

senpos commented 6 years ago

@joshuaskelly

A command is different from an event listener as commands are more mutable than event listeners. It's a very common use case for a chat moderator to add a new command during a stream.

I can see it now. Fair enough. But isn't opportunity to add commands on the fly requires some kind of external storage? Anyway, it is really cool thing.

senpos commented 6 years ago

@PythooonUser

How about adding a decorator for the add_command and on_command methods

I think, on_command will internally call add_command. Just like a neat shortcut.

But in this case, it is not clear, which command could be deleted (by the moderator from chat, for example) and which couldn't.