Implement context manager for bot storage

[PIG208]

The context manager is implemented based on a newly added storage calledCachedStorage. It is buffered storage that doesn't sync with the database until it's flushed. With CachedStorage, we can implement the context manager easily by loading all the data on enter and just flush all the modified (dirty) data on exit. This approach can help the user minimize the number of round-trips to the server almost invisibly (despite the fact that they need to use it with "with").

https://zulip.com/api/writing-bots#bot_handlerstorage

Manually tested with the incrementor bot.

Possible upcoming work: adding documentation and tests

679

[PIG208]

For some reason, I need to explicitly add BotStorage to SimpleStorage and CachedStorage to make mypy happy. I think it might be the problem with the @contextmanager decorator in the protocol. But I'm not sure what is the correct way to define such context manager in a protocol.

[PIG208]

Thank you for leaving the comments, I'm still trying to figure out what leads to the mypy errors, though everything works fine when I print-tested it. It would be the best if we don't have to inherit the protocol explicitly.

[LoopThrough-i-j]

Yeah, I am investigating the problem as well, will let you know if I find the solution.

[LoopThrough-i-j]

Can you remove the Inheritance of protocols and let the build fail here? This would help us investigate better.

[LoopThrough-i-j]

I think typing has a ContextManager return type you can refer to this.

[PIG208]

It appears that I have fixed the problem by using the recursive definition. Iterator[BotStorage] works just fine. Thank you for your help!

[LoopThrough-i-j]

No, problem. I pulled it to local and checked it myself. Looks great.

[andersk]

Changing the BotStorage protocol is a backwards-incompatible change because the server has its own implementation for EmbeddedBotHandler. We could do that, but I don’t think it’s justified in this case. You could just as easily have users write with CachedStorage(bot_handler.storage()) as storage: without changing the protocol.

[PIG208]

I have fixed the backward compatibility problem. But there is still space for optimization, like allowing the context manager to get and put all key-value pairs all at once instead of looping through the list with bot_storage.get. I couldn't think of a way to implement this improvement without changing the protocols.

[timabbott]

This makes sense, merged, thanks @PIG208! We can certainly change the protocol to do a single bulk operation down the line, we just need to coordinate carefully. (The EmbeddedBotHandler is AFAIK not in production use yet).

It's definitely worth doing tests and documentation updates to recommend this. Thanks @PIG208!

[PIG208]

Opened #693 for this