-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
RFC/1630 Signals #2042
RFC/1630 Signals #2042
Changes from 29 commits
a268087
25b170a
5548acb
c506f54
93371c3
93f87a1
1646a40
fbc5d25
e54d7c6
ded054f
63a50ba
6feb8e9
5b3cfba
d2fa5e4
6dbc830
b014b29
4a1343c
c83e7b1
555fb1d
470c9b5
14cdc7a
b04733e
53d2d70
ccbe4eb
4555b55
4a7252e
821e380
472d121
4917948
f0687bb
5c1e146
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,10 +1,16 @@ | ||||||||||||||||||||||||
from __future__ import annotations | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
import asyncio | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
from collections import defaultdict | ||||||||||||||||||||||||
from typing import Dict, Iterable, List, Optional | ||||||||||||||||||||||||
from typing import TYPE_CHECKING, Dict, Iterable, List, Optional, Set, Union | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
from sanic_routing.exceptions import NotFound # type: ignore | ||||||||||||||||||||||||
from sanic_routing.route import Route # type: ignore | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
from sanic.base import BaseSanic | ||||||||||||||||||||||||
from sanic.blueprint_group import BlueprintGroup | ||||||||||||||||||||||||
from sanic.exceptions import SanicException | ||||||||||||||||||||||||
from sanic.models.futures import FutureRoute, FutureStatic | ||||||||||||||||||||||||
from sanic.models.handler_types import ( | ||||||||||||||||||||||||
ListenerType, | ||||||||||||||||||||||||
|
@@ -13,6 +19,10 @@ | |||||||||||||||||||||||
) | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
|
||||||||||||||||||||||||
if TYPE_CHECKING: | ||||||||||||||||||||||||
from sanic import Sanic # noqa | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
|
||||||||||||||||||||||||
class Blueprint(BaseSanic): | ||||||||||||||||||||||||
""" | ||||||||||||||||||||||||
In *Sanic* terminology, a **Blueprint** is a logical collection of | ||||||||||||||||||||||||
|
@@ -21,7 +31,7 @@ class Blueprint(BaseSanic): | |||||||||||||||||||||||
|
||||||||||||||||||||||||
It is the main tool for grouping functionality and similar endpoints. | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
`See user guide | ||||||||||||||||||||||||
`See user guide re: blueprints | ||||||||||||||||||||||||
<https://sanicframework.org/guide/best-practices/blueprints.html>`__ | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
:param name: unique name of the blueprint | ||||||||||||||||||||||||
|
@@ -40,6 +50,7 @@ def __init__( | |||||||||||||||||||||||
version: Optional[int] = None, | ||||||||||||||||||||||||
strict_slashes: Optional[bool] = None, | ||||||||||||||||||||||||
): | ||||||||||||||||||||||||
self._apps: Set[Sanic] = set() | ||||||||||||||||||||||||
self.name = name | ||||||||||||||||||||||||
self.url_prefix = url_prefix | ||||||||||||||||||||||||
self.host = host | ||||||||||||||||||||||||
|
@@ -70,6 +81,14 @@ def __repr__(self) -> str: | |||||||||||||||||||||||
) | ||||||||||||||||||||||||
return f"Blueprint({args})" | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
@property | ||||||||||||||||||||||||
def apps(self): | ||||||||||||||||||||||||
if not self._apps: | ||||||||||||||||||||||||
raise SanicException( | ||||||||||||||||||||||||
f"{self} has not yet been registered to an app" | ||||||||||||||||||||||||
) | ||||||||||||||||||||||||
return self._apps | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
def route(self, *args, **kwargs): | ||||||||||||||||||||||||
kwargs["apply"] = False | ||||||||||||||||||||||||
return super().route(*args, **kwargs) | ||||||||||||||||||||||||
|
@@ -90,6 +109,10 @@ def exception(self, *args, **kwargs): | |||||||||||||||||||||||
kwargs["apply"] = False | ||||||||||||||||||||||||
return super().exception(*args, **kwargs) | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
def signal(self, event: str, *args, **kwargs): | ||||||||||||||||||||||||
kwargs["apply"] = False | ||||||||||||||||||||||||
return super().signal(event, *args, **kwargs) | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
@staticmethod | ||||||||||||||||||||||||
def group(*blueprints, url_prefix="", version=None, strict_slashes=None): | ||||||||||||||||||||||||
""" | ||||||||||||||||||||||||
|
@@ -132,6 +155,7 @@ def register(self, app, options): | |||||||||||||||||||||||
*url_prefix* - URL Prefix to override the blueprint prefix | ||||||||||||||||||||||||
""" | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
self._apps.add(app) | ||||||||||||||||||||||||
url_prefix = options.get("url_prefix", self.url_prefix) | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
routes = [] | ||||||||||||||||||||||||
|
@@ -200,6 +224,10 @@ def register(self, app, options): | |||||||||||||||||||||||
for listener in self._future_listeners: | ||||||||||||||||||||||||
listeners[listener.event].append(app._apply_listener(listener)) | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
for signal in self._future_signals: | ||||||||||||||||||||||||
signal.where.update({"blueprint": self.name}) | ||||||||||||||||||||||||
app._apply_signal(signal) | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
self.routes = [route for route in routes if isinstance(route, Route)] | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
# Deprecate these in 21.6 | ||||||||||||||||||||||||
|
@@ -209,3 +237,28 @@ def register(self, app, options): | |||||||||||||||||||||||
self.middlewares = middleware | ||||||||||||||||||||||||
self.exceptions = exception_handlers | ||||||||||||||||||||||||
self.listeners = dict(listeners) | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
async def dispatch(self, *args, **kwargs): | ||||||||||||||||||||||||
where = kwargs.pop("where", {}) | ||||||||||||||||||||||||
where.update({"blueprint": self.name}) | ||||||||||||||||||||||||
kwargs["where"] = where | ||||||||||||||||||||||||
await asyncio.gather( | ||||||||||||||||||||||||
*[app.dispatch(*args, **kwargs) for app in self.apps] | ||||||||||||||||||||||||
) | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
def event(self, event: str, timeout: Optional[Union[int, float]] = None): | ||||||||||||||||||||||||
events = set() | ||||||||||||||||||||||||
for app in self.apps: | ||||||||||||||||||||||||
signal = app.signal_router.name_index.get(event) | ||||||||||||||||||||||||
if not signal: | ||||||||||||||||||||||||
raise NotFound("Could not find signal %s" % event) | ||||||||||||||||||||||||
events.add(signal.ctx.event) | ||||||||||||||||||||||||
Comment on lines
+250
to
+255
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When this BP is registered on multiple apps, does every app need to have a signal in its router? In the current code this will throw an error if any of the apps doesn't have the signal. I think this an editing error, I think the correct logic is:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When you register a BP, all of its signals should be on the app. In the case where there are multiple apps, it should be on all of them. If it is missing from one, well, likely something went wrong somewhere. I am fine making this change, but it still feels to me like something bad just happened that should be handled here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, if it was your intention to do it that way, thats fine. It just looked like a copy&paste error when I was looking through the code, like that exception wasn't supposed to be there. (Eg, if the exception is thrown in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I went back and forth on this myself a couple times. I think maybe you are right though that the final exception doesn't quite fit. That exception would only be hit if there are no registered apps, in which caste the exception There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And, because we are accessing I suppose I was thinking that we needed to catch when Anyone see something else? |
||||||||||||||||||||||||
|
||||||||||||||||||||||||
if events: | ||||||||||||||||||||||||
return asyncio.wait( | ||||||||||||||||||||||||
[event.wait() for event in events], | ||||||||||||||||||||||||
return_when=asyncio.FIRST_COMPLETED, | ||||||||||||||||||||||||
timeout=timeout, | ||||||||||||||||||||||||
) | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
raise NotFound("Could not find signal %s" % event) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be nitpicking, but I am not so sure the
requirements
andwhere
complement each other so well. The names quite don't indicate what they are being used for when you read it for the first time.when
instead ofrequirements
andwith
instead ofwhere
might be a more suitable replacements ?@app.signal("foo.bar.baz", when={"one": "two"})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or may be
needs
andprovides
?@app.signal("foo.bar.baz", needs={"one": "two"})
Especially since
with
is a reserved keyword, though it has a nice ring to it, it will be bad idea :DThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. The name sort of stuck from the underlying implementation that is meant to be more generalized. I agree we could do a better job here with variable names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see these names changed too.
requirements
andwhere
are not intuitive to use.I do like
when
andwith
, it makes more sense in context, but would prefer not to use a keyword.I think
@app.signal()
should take something likewhere
orwhen
ormatches
, andapp.dispatch()
should have something likearguments
orwith
orprovides
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 They are both set to
where
right now.Let me first backup and explain. The
requirements
attribute on thesignal
is akin to what the router is using under the hood. Initially, its purpose is to help facilitate virtual host routing:At somepoint, I would like to expand its usage to also include routing based upon cookies, headers, etc. But there also might be a need for requirements to hold more structured meta data. I do not want to get too far ahead, but I also want to allow for flexibility to grow. Hence, it is a dictionary.
For now, we can simply pass through that whole object. At some point, maybe we need to add some logic.
In the current usage in signals, it adds the name of the blueprint to the object both with
signal
anddispatch
.So,
requirements
is a vestige of the underlying API. I agree that we can expose another term that makes more sense. However, whatever we choose I want to make sure is clear and unambiguous enough that someone can easily pick up the code and let it be obvious what is happening.And this is where I am torn. Originally my thought was that they needed to be two terms since the definition on the signal would potentially carry more details and meta information. On the
dispatch
it is just passing matching criteria.On the other hand, having two terms can ultimately be confusing. Especially if they are both beginning with
w
. I likewhere
because it is a familiar construct in query languages. I thinkmatches
andprovides
would be the types of keywords that someone would forget and need to look up every time they want to use it. Was itmatch
ormatches
? Does that go insignal
ordispatch
?I definitely am not a fan of a verb here. I still think
where
is easiest to remember. And, while I originally like having two names for future flexibility, I do not want to make a design decision on a feature that may or may not come into existence in the future.I guess that was a long way to say that I am in favor of a single argument still. If I had to rank the proposals:
needsprovidesmatchesarguments(confusing what this means, especially since we have another keyword that will provide arguments to the handlerThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sanic-org/core-devs What if we go with
condition
for both right now:Then, IF in the future we want to add more logic to the matching, we can add another kwarg:
FYI: Don't get bogged down in the exact API of
match
, it is just meant to be illustrative of the point that it could have a more structured future use case