Rfc/git 1630 signals #1902
Rfc/git 1630 signals #1902
Conversation
|
@ahopkins |
| @@ -146,7 +152,7 @@ def register_listener(self, listener, event): | |||
| :param event: when to register listener i.e. 'before_server_start' | |||
| :return: listener | |||
| """ | |||
|
|
|||
| subscribe(event_name=event, callback=listener, signals=self.signals) | |||
There was a problem hiding this comment.
If self.listener() decorator calls subscribe() (on line 141 above) then why does this helper function also need to call subscribe()? Wouldn't this result in two subscribes with the same listener and signals?
| def get( | ||
| self, uri, host=None, strict_slashes=None, version=None, name=None | ||
| ): | ||
| def get(self, uri, host=None, strict_slashes=None, version=None, name=None): |
There was a problem hiding this comment.
Looks like quite a lot of the changes in this file are formatting changes, was that just a result of merging in latest master? Or should these formatting changes be filtered out?
|
|
||
|
|
||
| def _extract_signal_namespace(event_name, signals): | ||
| global _CLASSIC_EVENT_ALIAS |
There was a problem hiding this comment.
can this be done without using a global variable?
| @@ -3,6 +3,9 @@ | |||
| from importlib import import_module | |||
| from inspect import ismodule | |||
|
|
|||
| from sanic.signals import Namespace | |||
There was a problem hiding this comment.
Looks like Namespace is only imported for Type checking, so has no purpose at runtime.
This import can changed to:
if typing.TYPE_CHECKING:
from sanic.signals import Namespace
Then in type declarations, change Namespace to 'Namespace' (string form)
| "request": Namespace(namespace="request", owner=self), | ||
| "response": Namespace(namespace="response", owner=self), | ||
| "middleware": Namespace(namespace="middleware", owner=self), | ||
| } |
There was a problem hiding this comment.
Looks like 4 Namespaces of signals are defined, but signal emitters are only implemented for server signals. Are the others planned to be implemented?
|
I think this is a really great potential win. I've been playing with the branch, and have some ideas on how to refine the implementation. I'll post on the RFC thread some changes I'm thinking about. One is to scrap the additional dependency and just roll our own signaling solution. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is incorrect, please respond with an update. Thank you for your contributions. |
|
Coming 2021 |
Take over from #1770 re: #1630