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

Allow route decorators to stack up again #1764

Merged
merged 3 commits into from
Jan 11, 2020

Conversation

ashleysommer
Copy link
Member

@ashleysommer ashleysommer commented Jan 10, 2020

A PR:#1690 broke the ability to stack route decorators.

This PR will allow again route decorators to stack up without causing a function signature inspection crash
Fix #1742

Also adds a test to detect this breakage, and verify its now fixed, and to ensure it stays fixed in the future.

@codecov
Copy link

codecov bot commented Jan 10, 2020

Codecov Report

Merging #1764 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1764   +/-   ##
=======================================
  Coverage   92.18%   92.18%           
=======================================
  Files          22       22           
  Lines        2240     2240           
  Branches      419      419           
=======================================
  Hits         2065     2065           
  Misses        136      136           
  Partials       39       39
Impacted Files Coverage Δ
sanic/response.py 100% <100%> (ø) ⬆️

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 fdaf70b...d6d5f9e. Read the comment docs.

sanic/app.py Outdated
@@ -194,6 +194,10 @@ def route(
strict_slashes = self.strict_slashes

def response(handler):
if isinstance(handler, (tuple, list)):
Copy link
Member

Choose a reason for hiding this comment

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

Can we add some comments here for explaining?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment explaining it

sanic/app.py Outdated
@@ -205,7 +209,7 @@ def response(handler):
if stream:
handler.is_stream = stream

routes = self.router.add(
new_routes = self.router.add(
Copy link
Member

Choose a reason for hiding this comment

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

new_routes seems a little bit weird.. maybe we can just extend in place.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to inplace extend.

Add test for double-stacked websocket decorator
remove introduction of new variable in route wrapper, extend routes in-place.
Add explanation of why a handler will be a tuple in the case of a double-stacked route decorator
@ashleysommer
Copy link
Member Author

Pushed some changes, added the same fix for @websocket() routes too, added a new test for double-stacked websocket routes.

Copy link
Member

@yunstanford yunstanford left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@yunstanford yunstanford merged commit b565072 into sanic-org:master Jan 11, 2020
@ashleysommer ashleysommer deleted the multi-route-stack branch May 14, 2020 01:02
ashleysommer added a commit to ashleysommer/sanic that referenced this pull request May 14, 2020
@ashleysommer ashleysommer restored the multi-route-stack branch July 15, 2020 12:49
ahopkins added a commit that referenced this pull request Jul 29, 2020
* Cherry pick PRs to backport to 19.12LTS

Includes commits from:
#1762
#1764
#1789

* Fix type annotation issue; run black and isort

* Update Makefile

Co-authored-by: Ashley Sommer <[email protected]>
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.

Changes to Blueprints application breaks multiply applied routes!!!
2 participants