Room-11 / Jeeves

Chatbot for Stack Overflow
MIT License
42 stars 19 forks source link

Allows Jeeves to reply to it's own commands when dev mode is enabled. #257

Closed Nessworthy closed 6 years ago

Nessworthy commented 7 years ago

Useful for testing on the same account as the bot.

DaveRandom commented 7 years ago

If this is necessary it feels like a regression, I'm sure @ekinhbayar did this once, although I can't find it anywhere :-/

This should be a separate option though IMO, e.g. $config['dev-mode']['respond-to-self'] - dev-mode is designed as a map to enable stuff like this, it's just that it only has the one enable option at the moment. When I have my local in dev mode I still don't want it to respond to itself :-P

Nessworthy commented 7 years ago

Makes sense, I'll give it a shake!

DaveRandom commented 7 years ago

@Nessworthy it might be worth replacing the bool argument with some kind of "dev options" VO, i.e. something like:

class DevOptions
{
    private $enabled;
    private $respondToSelf;
    public function __construct(bool $enabled, bool $respondToSelf)
    {
        $this->enabled = $enabled;
        $this->respondToSelf = $respondToSelf;
    }
    public function isEnabled()
    {
         return $this->enabled;
    }
    public function shouldRespondToSelf()
    {
        return $this->enabled && $this->respondToSelf;
    }
}

... and where you are checking for it:

if ($this->devOptions->shouldRespondToSelf() && ...)

I'm not 100% sure about this approach though /cc @PeeHaa ?

I like the idea of the behavioural descriptions, and the fact that by wrapping it in methods the "enable" toggle is taken care of, so a consumer doesn't need to remember to check both.

PeeHaa commented 7 years ago

Yes. this has to be a regression.

@ekinhbayar implement the dev mode which should do what this PR aims to do.
Before knowing what the best way to solve this is I would like to know how the regression happened.

DaveRandom commented 7 years ago

@PeeHaa the history is hard to read because I am a terrible person, but it looks like I probably screwed this up during the StackChat extraction/refactor, again because I am a terrible person.

The tl;dr is that I am terrible person.

ekinhbayar commented 7 years ago

I can't find it either :P thinking we used to have the websocket handler on Jeeves and this piece of code was there, so I'm guessing it was lost when StackChat emerged indeed. It doesn't really matter, this is the missing code so let's just merge it? :-)

Edit: actually if we are in need of more options I kinda like the idea of a VO for it.

PeeHaa commented 6 years ago

I like the idea of having a DevOptions like object available.

If we still want it create a dedicated issue. Merged this PR.

tnx @Nessworthy