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

wraps websocket handler #1696

Closed
wants to merge 6 commits into from
Closed

Conversation

andribas404
Copy link

Add wraps to websocket_handler.
Without it, all routes_names for websockets are identical and do not keep blueprint name.

Signed-off-by: Andrey Petukhov [email protected]

Signed-off-by: Andrey Petukhov <[email protected]>
@codecov
Copy link

codecov bot commented Oct 10, 2019

Codecov Report

Merging #1696 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1696   +/-   ##
=======================================
  Coverage   92.17%   92.17%           
=======================================
  Files          23       23           
  Lines        2312     2313    +1     
  Branches      424      424           
=======================================
+ Hits         2131     2132    +1     
  Misses        141      141           
  Partials       40       40           
Impacted Files Coverage Δ
sanic/app.py 92.27% <100.00%> (+0.01%) ⬆️

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 7c04c9a...060e3b8. Read the comment docs.

Signed-off-by: Andrey Petukhov <[email protected]>
@andribas404
Copy link
Author

Greetings,
probably we could split this PR to 2 pull requests:

  • one that adds first test to @wraps method
  • and second with tests for strict_slashes logic:
  1. if strict_slashes is True, url must end with slash for "/route" and "/route/"?
  2. url_for should always return valid url that result to response status code 200, not 404.
    I think this statements needs clarifications from the maintainer.

Thank you for Sanic project.

@Tronic
Copy link
Member

Tronic commented Oct 14, 2019

I'd like to see a migration path for getting rid of the strict_slashes setting. The only sensible option in this regard is (1) if a route ends with slash, redirect GETs and 404 anything else trying to access without slash, (2) if a route does not end with slash, 404 anything with slash, or (3) if matching routes exist with and without slash, match strictly the one used in request.

Those wishing for non-strict functionality are free to rely on option 3 and explicitly specify two routes. The question is how to get there without breaking existing apps too badly.

@ahopkins
Copy link
Member

@Tronic I am with you on that. But, I think it needs to be part of a larger discussion that resolves a long list of items that need to be resolved with the router. I know I have been saying this for a while and there has been a lot of tangential talk about it, but I think 2020 should finally be the time when we tackle this item.

Andrey Petukhov added 3 commits January 24, 2020 09:01
Signed-off-by: Andrey Petukhov <[email protected]>
Signed-off-by: Andrey Petukhov <[email protected]>
@andribas404
Copy link
Author

Sorry for impatience, changed pull request according to Single Responsibility Principle.

@Tronic
Copy link
Member

Tronic commented Mar 29, 2020

Tagging for 20.3.

@Tronic Tronic mentioned this pull request Mar 30, 2020
@Tronic Tronic added the needs review the PR appears ready but requires a review label Jun 21, 2020
@Tronic Tronic removed the needs review the PR appears ready but requires a review label Jun 28, 2020
@ahopkins
Copy link
Member

Conflicts with #1853. Need to move the @wraps. I will take a look at this later.

@ahopkins
Copy link
Member

Closing in favor of #1880

@ahopkins ahopkins closed this Jun 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants