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 a if-statement at build_spec checking add default 200 response or not. #116

Merged
merged 3 commits into from
Jul 2, 2019

Conversation

jackey8616
Copy link
Contributor

The if-statement is to check a responses is empty or not to decide add a default 200 response or not.

The if-statement is to check a responses is empty or not to decide add a de-
fault 200 response or not.
Copy link
Member

@chenjr0719 chenjr0719 left a comment

Choose a reason for hiding this comment

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

Hi @jackey8616,

First, thanks for your contribution, here are something I need your help to merge this PR:

  1. You change the default behavior of adding default 200 response to swagger. This is a good point, but you forget to modify the current tests. Could you fix the tests? About how to run tests at local development you can check this to get more information.

  2. This project uses black as default coding style. Could you also apply it? Here are some instructions.

BTW, I've noticed that you are from Taiwan and study in Yuntech. So was I. XD

Fix test case at test_response with removing default 200 status code.
But test_swagger_endpoint_redirect still have a status code 302, not sure what
 this for.
@jackey8616
Copy link
Contributor Author

@chenjr0719
I have fixed the test case of default 200 status code and also fit black codestyle.
But there is still a test case with 302 status code of test_swagger_endpoint_redirect method.
I'm no sure what is that for.
Please review.

And, what a small world. lol. 😆

@chenjr0719
Copy link
Member

@jackey8616
It looks like some conflicts. Would you mind to solve it, please?
About the redirect, you can check this to get more information.

@codecov-io
Copy link

codecov-io commented Jul 1, 2019

Codecov Report

Merging #116 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
+ Coverage   95.43%   95.45%   +0.02%     
==========================================
  Files           4        4              
  Lines         438      440       +2     
  Branches       93       94       +1     
==========================================
+ Hits          418      420       +2     
  Misses          6        6              
  Partials       14       14
Impacted Files Coverage Δ
sanic_openapi/swagger.py 95.08% <100%> (+0.08%) ⬆️

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 d03fd00...e624f44. Read the comment docs.

@jackey8616
Copy link
Contributor Author

@chenjr0719 ,
Suddenly, everything is good and charmy...

@chenjr0719 chenjr0719 merged commit bf4667e into sanic-org:master Jul 2, 2019
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.

None yet

3 participants