-
-
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
GIT-1630: add basic signal infra #2041
GIT-1630: add basic signal infra #2041
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2041 +/- ##
=============================================
- Coverage 92.070% 91.605% -0.465%
=============================================
Files 33 35 +2
Lines 3102 3228 +126
Branches 542 556 +14
=============================================
+ Hits 2856 2957 +101
- Misses 166 186 +20
- Partials 80 85 +5
Continue to review full report at Codecov.
|
def signal(self, signal: str): | ||
def _wrapper(handler: t.Callable): | ||
parts = signal.split(".") | ||
if len(parts) < 2 or len(parts) > 3: |
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.
May be we can avoid checking and raise an Index error and be done with it?
I have a branch that implements this using the new router. Some slight changes in API design, but I think a similar approach. I will push the branch for you to look at and take a closer look at this PR later. Ping me on the DIscord server, it might be nice to find a chance to hash out some details in realtime. |
See signals-api |
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.
Need to discuss implementation strategy.
Closing in favor of /pull/2042 |
@ahopkins / @sanic-org/sanic-core-devs This is in no way an attempt to discard the PR you already have in place, but during my break from all the community, I had a different implementation in mind than my original DRAFT. The migration to
sanic-router
made that way more easier. The whole idea of Mixin is coming to the resume incredibly well.This is still an early draft of the changes and if you don't mind, I would like to pick this back up in the coming days as I am trying to get back to the community and a familiar implementation requirement might be a good thing to start with.
TODO
Router.get
might enable much better route even generation?