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

Refactor message handling again #671

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

Askaholic
Copy link
Collaborator

This is something I've been pondering for a while, and have recently learned enough about python internals to figure out how to do it!

Instead of mapping messages to handlers by using string formatting on attribute names, or by explicitly listing every handler in a giant dict, the mapping is generated automatically at import time using a metaclass. It also doesn't use a single key mapping, but a search tree which is able to match an arbitrary number of subcommands.

To create a connection:

from server.core.connection import Connection, handler


class MyConn(Connection):
    @handler("foo")
    async def handle_foo(self, message):
        print("foo")

    @handler("foo", bar="baz")
    async def handle_foo_bar(self, message):
        print("foobar")

    @handler(bar="baz")
    async def handle_bar_baz(self, message):
        print("barbaz")


conn = MyConn(proto, addr)
await conn.on_message_received({"command": "foo"})  # foo
await conn.on_message_received({"command": "foo", "bar": "qux"})  # foo
await conn.on_message_received({"command": "foo", "bar": "baz"})  # foobar
await conn.on_message_received({"bar": "baz"})  # barbaz

One thing I still want to do in the future is figure out a nice way to modularize the command handlers so they can be split across multiple files. For instance we could put all of the "admin" commands in one file and all of the "social" commands in another. Right now it could be done with subclasses, but that doesn't seem like the most elegant solution to me.

I also refactored the service registration so it no longer needs a metaclass, and supports overriding the implicitly derived service name like this:

from server.core import Service 


class MyService(Service, name="my_thing"):
    pass


class FooService(Service):
    def __init__(self, my_thing):
        assert isinstance(my_thing, MyService)

Copy link
Member

@cleborys cleborys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Phew, that was long 😅 But very cool.

When introducing such linked concepts like SearchTree I always get afraid someone will at some point create loops, but you really have to abuse it to do that here, so i think it's safe 😝

Comment on lines +24 to +27
if "router" not in namespace:
namespace["router"] = Router("command")

router = namespace["router"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't these just dicts and the following works?

Suggested change
if "router" not in namespace:
namespace["router"] = Router("command")
router = namespace["router"]
router = namespace.get("router", Router("command"))

handler_name = self.router.dispatch(message)
except RouteError:
# We use type(self) because we want to properly follow the MRO
handler_name = super(type(self), self).router.dispatch(message)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand this correctly as:

  1. check your own router for a handler
  2. if not found, check your parent's router for a handler
  3. if not found, leave the RouteError raised by your parent's router unhandled.

In particular, if there is no 4. propagating up to your grandparent's router, why call super rather than ConnectionMeta directly?
I also don't understand what "MRO" means, so maybe that's the issue here :P

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh maybe you want to inherit from Connection and then check ChildConnection and Connection's router only? Still seems slightly weird to only check on parent rather than recursively call super(type(self), self).dispatch(message) plus base case handling.

:raises: RouteError if no handler was found
"""
handler_func = self.dispatch(message)
return await handler_func(message)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So all handlers need to be async?(!)

"""
Send multiple messages in the form of a list of dictionaries.

May be more optimal than sending a single message.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"more optimal" 😝

Comment on lines +87 to +89
with contextlib.suppress(KeyError):
return self[message]
return None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You seem to favor these instead of try/except now?


from server.core import Service


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also test that the snake case conversion works if it's not tested yet, but that feels very minor.

Comment on lines +30 to +31
foo.reset_mock()
bar.reset_mock()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or split it into two tests

Comment on lines +32 to +33
with pytest.raises(RouteError):
router.dispatch({"command": "qux"})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also test router.dispatch({"not_command": "foo"}) ?

@Askaholic Askaholic force-pushed the develop branch 18 times, most recently from b13ea36 to c621626 Compare December 28, 2020 04:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants