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

Consider adding "on_command" decorator #61

Open
senpos opened this issue Jul 7, 2018 · 9 comments
Open

Consider adding "on_command" decorator #61

senpos opened this issue Jul 7, 2018 · 9 comments

Comments

@senpos
Copy link
Contributor

senpos commented Jul 7, 2018

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
Copy link
Owner

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
Copy link
Contributor Author

senpos commented Jul 8, 2018

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
Copy link
Owner

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

@PythooonUser
Copy link
Collaborator

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
Copy link
Contributor Author

senpos commented Jul 8, 2018

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
Copy link
Owner

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
Copy link
Collaborator

PythooonUser commented Jul 9, 2018

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
Copy link
Contributor Author

senpos commented Jul 9, 2018

@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
Copy link
Contributor Author

senpos commented Jul 9, 2018

@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.

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

No branches or pull requests

3 participants