Refactor

[kmkurn]

This is a big refactor. I'll try to summarise the changes as best as I can.

1. Use manage.py to unify all scripts invocation. Previously, we have to invoke flake8 and py.test to run linters and tests. Now we have ./manage.py lint and ./manage.py test. This is easier to memorise and all configuration can be done by the script instead of manually doing it before running commands. manage.py also provides two useful commands, namely ./manage.py runserver and ./manage.py shell, to run server locally and start a shell session with customised context.
2. Configuration variables are now stored in .env file and tests/.env for tests. This is done so that @wazaundtechnik 's suggestion to use Honcho in local can be done. I think it would be useful if we can exactly simulate the production environment.
3. Reorganise code. Previously, we put everything under tululbot.py. Now things are put under its module. Tests are also separated (hopefully) logically.
4. Due to code reorganisation, some tests are failing. I fixed some of them and marked the others as expected to fail. I hope the original author can help me fix those tests.
5. Provide a better way to create bot commands, as discussed in #23.
6. Refactor tests, as discussed in #22.
7. Update README, update comments, and write docstrings and doctests.

I think that pretty much sums up the changes. Should you have any questions, please let me know. Feedbacks are welcome.

[kmkurn]

Some things I particularly need feedback about:

1. flake8 has support for mccabe plugin. It's basically to give error when our code's cyclomatic complexity exceeds some threshold. I think this is useful to keep our code simple but I'd like to hear others' opinion on this.
2. flake8 also has support for VCS hook. Basically it will be run automatically before commit. This seems like a good idea but I am not too sure. What do you think?
3. @wazaundtechnik suggested that we need a single command to run both linters and tests. I can't come up with a good name for the command so can anyone suggest a name?
4. What do you guys think about automatically generating documentation? This can be done with sphinx.

Thanks.

[fushar]

Thanks for the refactor!

I would like to comment on the overall design.

• The class TululBot is actually not a "TululBot", but rather than a, something like, "AbstractTelegramBot" since it only defines mechanism for plugging in bot commands.
• That implies that "actual TululBot" is actually a Flask app, which instantiates the "AbstractTelegramBot", add tulul commands to it, and transfer request messages to it.

Is my above perception correct?

I think we can do better:

• The class TululBot should already consist of all tulul commands.
• The class TululBot should not be tied to Flask or any framework.
• Also, I think the commands are such strong entities that it is not appropriate to call them "utils" (you are putting them in a file called util.py).

Any thoughts?

[wazaundtechnik]

Hi @kemskems, this is a great refactor. Big thanks!

Hi @fushar, could you draw a simple uml diagram for this? I think we should agree on the design first.

[kmkurn]

@fushar @wazaundtechnik Thanks for the feedback :-)

@fushar Yes, I think your perception is accurate.

The class TululBot should already consist of all tulul commands.

At first, I thought this is impossible since all tulul commands can only be performed under the context of an update. Then I realised I can initialise TululBot by passing the update object to it. So I think maybe we can do like:

1. TululBot class has all tulul commands as its methods. Let's call this the new TululBot.
2. The old TululBot class is modified to something like Dispatcher class that is used only to register commands, using the new TululBot's methods as the callbacks.
3. When an update arrives, the main view will create a TululBot object (passing the update object to the constructor), create a Dispatcher object, register all TululBot's object methods as commands, and run the command.

Is this what you mean?

The class TululBot should not be tied to Flask or any framework.

Yes. The current TululBot does not care at all about Flask or any other frameworks.

Also, I think the commands are such strong entities that it is not appropriate to call them "utils" (you are putting them in a file called util.py).

Which commands? Right now the actual commands are put inside the main view. If what you mean is TululBot class, then what do you suggest? Please don't say TululBot.py, there's no reason to put only a class in a file :-P

@wazaundtechnik Please remind me to implement custom 500 handler to ease debugging in production.

[kmkurn]

Almost forgot. Can anyone give their opinion on this?

Some things I particularly need feedback about:

1. flake8 has support for mccabe plugin. It's basically to give error when our code's cyclomatic complexity exceeds some threshold. I think this is useful to keep our code simple but I'd like to hear others' opinion on this.
2. flake8 also has support for VCS hook. Basically it will be run automatically before commit. This seems like a good idea but I am not too sure. What do you think?
3. @wazaundtechnik suggested that we need a single command to run both linters and tests. I can't come up with a good name for the command so can anyone suggest a name?
4. What do you guys think about automatically generating documentation? This can be done with sphinx.

Thanks.

[kmkurn]

@fushar Forget everything I said about initialising TululBot object by passing update object into it. I was stuck with the idea that a bot must return a JSON response that is compliant with Telegram's sendMessage API. While in fact, it doesn't have to. The bot can simply return the text to be sent and let the main view wrap it as a response.

So basically my proposal (which hopefully agrees with your suggestion) is like this:

1. The currently TululBot class is renamed to CommandDispatcher class. The functionality is not altered; we can register a bot command pattern and its callback/handler and we can invoke its run_command. This class is stored under decorators module because the recommended way to register pattern & callback is by using the provided decorator.
2. Create a commands module, which acts like your suggested TululBot class. Within it, tulul commands are defined as functions and they return a simple text. It doesn't know anything about updates, sending messages, etc. The reason why not creating a class is because there are no shared states among commands right now so a class is unnecessary.
3. When an update arrives, the main view will parse it, ask the bot to run the command, and finally wrap the returned text as a JSON response and send it.

The benefit of this approach is that we can register commands just once at the beginning, not on every update like now.

What do you think?

[fushar]

@kemskems The proposal looks good to me and agrees with what I thought. One question: where/when exactly will you register the commands to the dispatcher?

1. flake8 has support for mccabe plugin. It's basically to give error when our code's cyclomatic complexity exceeds some threshold. I think this is useful to keep our code simple but I'd like to hear others' opinion on this.
2. flake8 also has support for VCS hook. Basically it will be run automatically before commit. This seems like a good idea but I am not too sure. What do you think?
4. What do you guys think about automatically generating documentation? This can be done with sphinx.

Those are good suggestions but I think they should be separate issues and are not incorporated with this refactor.

@wazaundtechnik suggested that we need a single command to run both linters and tests. I can't come up with a good name for the command so can anyone suggest a name?

If I'm not mistaken, Gradle Java plugin uses check for this purpose; maybe we can use that name too.

[mufid]

Why you don't consider linting is also a test?

[kmkurn]

Thanks @fushar.

One question: where/when exactly will you register the commands to the dispatcher?

Inside commands module. Here's what I have in mind:

# In commands.py
from .decorators import CommandDispatcher

dispatcher = CommandDispatcher()

@dispatcher.command(r'^/leli (?P<term> .+)$')
def leli(term):
    # Do something with term here

# Other commands definition follow

# In tululbot/__init__.py
from .commands import dispatcher

@app.route('/')
def main():
    # Parse incoming updates
    reply_text = dispatcher.run_command(text)
    return reply(reply_text)

Some commands may return a tuple, where the second component is a boolean whether to disable preview or not.

@wazaundtechnik because 1) testing and linting are different, and 2) if test includes lint, then what if one only wants to run the lint? She has to use flake8 directly. That kind of defeats the purpose of unifying all commands under manage.py.

[fushar]

Please see my inline comments. Other than those, the reset LGTM.

[kmkurn]

Thanks @fushar for the code review. Just updated this PR again. Please check.

[fushar]

+1, really good!

[kmkurn]

@fushar Thanks :smile:. If no one else wants to review, I'm gonna merge this.