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

Add signal reservations #2060

Merged
merged 5 commits into from
Mar 14, 2021
Merged

Add signal reservations #2060

merged 5 commits into from
Mar 14, 2021

Conversation

ahopkins
Copy link
Member

@ahopkins ahopkins commented Mar 14, 2021

It occurred to me while writing documentation for #1630/#2042 that there were certain patterns of names that we probably do not want people to start declaring on their own. We do not know exactly what those patterns are yet, but I thought it might make sense to lock in some reserved names so people do not start implementing them now, and then end up with weird behaviors down the road.

Basically, that means we are adding some code right now that will be temporary. Or, at the very least will change.

Perhaps I am being overly cautious. Thoughts? Do we need this? Is the list complete enough for now?

Current reservations:

  • Anything starting server.
  • Anything starting http.
  • Anything starting sanic. that is not sanic.notice. or sanic.log.

@ahopkins ahopkins requested a review from a team as a code owner March 14, 2021 10:04
@codecov
Copy link

codecov bot commented Mar 14, 2021

Codecov Report

Merging #2060 (2a0fe66) into master (d4660d0) will increase coverage by 0.014%.
The diff coverage is 100.000%.

Impacted file tree graph

@@              Coverage Diff              @@
##            master     #2060       +/-   ##
=============================================
+ Coverage   92.176%   92.190%   +0.014%     
=============================================
  Files           38        38               
  Lines         3451      3457        +6     
  Branches       580       581        +1     
=============================================
+ Hits          3181      3187        +6     
  Misses         184       184               
  Partials        86        86               
Impacted Files Coverage Δ
sanic/signals.py 93.056% <100.000%> (+0.631%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4660d0...2a0fe66. Read the comment docs.

@ashleysommer
Copy link
Member

that is not sanic.notice. or sanic.log.

Is the thought that the user might want to handle potential future sanic.notice.* and sanic.log.* events themselves?

sanic/signals.py Outdated Show resolved Hide resolved
sanic/signals.py Outdated
Comment on lines 164 to 165
if reservation.reference is None or reservation.action is None:
continue
Copy link
Member

@ashleysommer ashleysommer Mar 14, 2021

Choose a reason for hiding this comment

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

reservation.reference cannot be None here. We call reservation.reference.split(",") above, so it must be a string. It it was None, Line 158 would throw and exception.
Do we need to move if reservation.reference is None: continue to above line 155?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Again, maybe I am overcomplicating and the whole thing should just be:

parts = event.split(",")
if parts[0] in ("server", "http", "sanic"):
    raise InvalidSignal

And then worry about any more complicated logic when we get to it.

Copy link
Member

Choose a reason for hiding this comment

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

Yep. I'd go with the simpler solution for now. We can think about reservation rules and how they are matched against signals, in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I updated it to only be reserved namespaces http and server.

I want to push people (via the docs) to get used to using sanic.notice.something when they have a simple use case and a full blown namespaced event doesn't make sense.

@ahopkins
Copy link
Member Author

that is not sanic.notice. or sanic.log.

Is the thought that the user might want to handle potential future sanic.notice.* and sanic.log.* events themselves?

I think these might be names we want to control. Probably also sanic.exception.*. But, perhaps this is too much limitation. Maybe we just disallow them from defining anything that is server, http , or sanic and leave it at that.

@ahopkins ahopkins merged commit 2fea954 into master Mar 14, 2021
@ahopkins ahopkins deleted the signal-reservation branch March 14, 2021 13:22
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