-
-
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
Merged
Merged
RFC/1630 Signals #2042
Changes from all commits
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
a268087
Temp working version of initial signal api
ahopkins 25b170a
Merge branch 'master' into signals-api
ahopkins 5548acb
Add signals testing
ahopkins c506f54
fix signals router finalizing
ahopkins 93371c3
merge conflict
ahopkins 93f87a1
Use new sanic-router to resolve app/bp on same signal path
ahopkins 1646a40
Additional tests
ahopkins fbc5d25
Add event test
ahopkins e54d7c6
finalize test
ahopkins ded054f
remove old comment
ahopkins 63a50ba
Add some missing annotations
ahopkins 6feb8e9
multiple apps per BP support
ahopkins 5b3cfba
Merge branch 'master' of github.com:sanic-org/sanic into signals-api
ahopkins d2fa5e4
deepsource?
ahopkins 6dbc830
rtemove deepsource
ahopkins b014b29
nominal change
ahopkins 4a1343c
fix blueprints test
ahopkins c83e7b1
Merge branch 'master' into signals-api
ahopkins 555fb1d
Merge branch 'master' into signals-api
ahopkins 470c9b5
trivial change to trigger build
ahopkins 14cdc7a
Merge branch 'signals-api' of github.com:sanic-org/sanic into signals…
ahopkins b04733e
signal docstring
ahopkins 53d2d70
Updates from feedback
ahopkins ccbe4eb
squash
ahopkins 4555b55
squash
ahopkins 4a7252e
Add a couple new tests
ahopkins 821e380
Merge branch 'master' into signals-api
ahopkins 472d121
Add some suggestions from review
ahopkins 4917948
Merge branch 'signals-api' of github.com:sanic-org/sanic into signals…
ahopkins f0687bb
Remove inaccessible code
ahopkins 5c1e146
Change where to condition
ahopkins File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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:
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.
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 comment
The 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
for
loop, there is no way for theNotFound
exception at the end of the function to be hit).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 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
NotFound
doesn't make sense.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.
And, because we are accessing
self.apps
as a property, you would get an exception earlier than that before it ever reaches there.I suppose I was thinking that we needed to catch when
events
was empty, but I am not seeing how it could be.Anyone see something else?