mrshu / brutal-plugins

A set of plugins for brutal, the mighty chatbot
Apache License 2.0
4 stars 2 forks source link

sup: know the info about current day #103

Closed Adman closed 9 years ago

Adman commented 9 years ago

Signed-off-by: Adrian Matejov a.matejov@centrum.sk

Adman commented 9 years ago

@mrshu @pepol

pepol commented 9 years ago

LGTM, any comments @mrshu?

mrshu commented 9 years ago

I do not think I like the fact that we will include calendar_sk.py among plugins even though there is no brutal plugin code in it. In my book this should be its own module, preferably on pipy or somewhere else, so that it can be include in requirements.txt.

When we came up with the idea for sup it was centered around the fact that you would use it as a default ping to any of your bots, and your bots would respond with responses from given commands. The whole sup command would in the end become just one big alias. This is based on the idea that separate commands are small, well tested (huh) and can be built upon.

@Adman I am not sure whether you share my view on this but I believe this discussion has to happen somewhere.

For now though, except for that module it LGTM.

Adman commented 9 years ago

@mrshu yeah, you are right. Sup command should call other commands and return the message joined with commas for instance. I shoud probably rename this plugin to !dayinfo . That would suit the purpose of this plugin.

I disagree with the idea that calendar_sk should be a module. It's something like config file. I recommend creating a folder where each plugin would handle its own config file for stuff like these ones.

mrshu commented 9 years ago

@Adman I do not see how calendar_sk is something like a config file but maybe I am looking at it from a wrong angle.

Can you provide a little more context?

mrshu commented 9 years ago

@Adman ping

Adman commented 9 years ago

@mrshu Going to make it as a module

Adman commented 9 years ago

@mrshu bump,

Adman commented 9 years ago

@mrshu bump

mrshu commented 9 years ago

@Adman apart from that one comment, looks good.

Adman commented 9 years ago

@mrshu I added the option for specifying date. If everything is okay, I will squash all the commits

mrshu commented 9 years ago

@Adman I do not see any issues, feel free to squash it and I'll merge it.

Thanks!

Adman commented 9 years ago

@mrshu Should be done