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

Bug fix for host parameter issue with lists #1776

Merged
merged 4 commits into from
Jun 28, 2020
Merged

Conversation

damianj
Copy link
Contributor

@damianj damianj commented Jan 30, 2020

As explained in #1772 there is an issue when using a list as an argument for the host parameter in the Blueprint.route() decorator. I've traced the issue back to this line, and the if conditional should ensure that the name attribute isn't accessed when route is None.

As explained in sanic-org#1772 there is an issue when using a list as an argument for the host parameter in the Blueprint.route() decorator. I've traced the issue back to this line, and the if conditional should ensure that the name attribute isn't accessed when route is None.
@codecov
Copy link

codecov bot commented Jan 30, 2020

Codecov Report

Merging #1776 into master will increase coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1776      +/-   ##
==========================================
+ Coverage   91.53%   91.63%   +0.09%     
==========================================
  Files          27       27              
  Lines        3000     3000              
  Branches      545      545              
==========================================
+ Hits         2746     2749       +3     
+ Misses        174      172       -2     
+ Partials       80       79       -1     
Impacted Files Coverage Δ
sanic/blueprints.py 95.94% <100.00%> (ø)
sanic/server.py 74.33% <0.00%> (+0.66%) ⬆️

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 d81096f...76c5839. Read the comment docs.

Copy link
Member

@ahopkins ahopkins left a comment

Choose a reason for hiding this comment

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

This seems like an okay change. But, it might be nice to have a test with host routing with a string and a list of strings.

Thanks for the PR.

@damianj
Copy link
Contributor Author

damianj commented Jan 31, 2020

@ahopkins I think the tests are being addressed in #1775? Unless that is not addressing Blueprint.route() tests.

harshanarayana
harshanarayana previously approved these changes Jan 31, 2020
@damianj
Copy link
Contributor Author

damianj commented Feb 2, 2020

@ahopkins I added some unit tests for using lists with the host parameter. There were already some for testing strings with the host parameter.

@sjsadowski
Copy link
Contributor

@ahopkins can you resolve your change request if you think that we've met your threshhold?

@damianj
Copy link
Contributor Author

damianj commented Feb 12, 2020

Just following up to see if there is anything else I can or need to do in order to get this merged?

@Tronic Tronic mentioned this pull request Mar 30, 2020
@stale
Copy link

stale bot commented May 13, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is incorrect, please respond with an update. Thank you for your contributions.

@stale stale bot added the stale label May 13, 2020
@ahopkins ahopkins removed the stale label May 13, 2020
@Tronic
Copy link
Member

Tronic commented Jun 21, 2020

This seems like an okay change. But, it might be nice to have a test with host routing with a string and a list of strings.

Thanks for the PR.

#1775 was closed (stale) and this now has tests. Can we resolve this change request and get this merged?

@ahopkins ahopkins merged commit cf9ccda into sanic-org:master Jun 28, 2020
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.

5 participants