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

Remove prefix from websocket handler name #2020

Closed

Conversation

laggardkernel
Copy link

@laggardkernel laggardkernel commented Feb 5, 2021

Update: opened for wrong branch, reopened another pr.


Remove the websocket prefix websocket_handler_ introduced in
761eef7. It makes url_for() unable to find the corresponding ws route.

@app.websocket('/ws')
async def websocket(request, ws):
    return ""

The key for the above handler used in Router.route_names is websocket_handler_websocket, not websocket. Removing the websocket_handler_ prefix fixes this bug. Searching in the source code, this very prefix is not used in anywhere.

I guess the contributor of commit 761eef7 tried to mimic the registration behavior of static file handler, which prepend the handler name with prefix _static_, in order to store static file handler separately in Router.routes_static_files, not in Router.routes_names.

Remove the websocket prefix "websocket_handler_" introduced in
761eef7. Makes url_for() unable to find the corresponding ws route.
@codecov
Copy link

codecov bot commented Feb 5, 2021

Codecov Report

Merging #2020 (e857768) into master (ffca01f) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2020   +/-   ##
=======================================
  Coverage   91.45%   91.45%           
=======================================
  Files          28       28           
  Lines        3171     3171           
  Branches      571      571           
=======================================
  Hits         2900     2900           
  Misses        192      192           
  Partials       79       79           
Impacted Files Coverage Δ
sanic/app.py 94.91% <100.00%> (ø)

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 ffca01f...e857768. Read the comment docs.

@ahopkins
Copy link
Member

ahopkins commented Feb 5, 2021

Thanks, this is awesome and I appreciate it. But there's another PR that is a complete overhaul of the Router and the api for url_for is also getting an update. This is included.

@ahopkins
Copy link
Member

ahopkins commented Feb 5, 2021

Maybe you can change this as a PR to the 20.12 LTS branch and we can include it backwards there.

@laggardkernel
Copy link
Author

laggardkernel commented Feb 5, 2021

Deleted. Comment moved to another PR.

@laggardkernel
Copy link
Author

Maybe you can change this as a PR to the 20.12 LTS branch and we can include it backwards there.

Thx. Opened another PR for 20.12LTS.

@vltr
Copy link
Member

vltr commented Feb 11, 2021

2020 was never going to happen, anyway 🤣

@ahopkins
Copy link
Member

😷

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.

3 participants