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 eof method to close stream #2094

Conversation

ajaygupta2790
Copy link
Contributor

@ajaygupta2790 ajaygupta2790 commented Mar 29, 2021

Resolves #2067

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.

Looks good. Can you add some tests that utilize the new style streaming response?

I would maybe look in tests/test_response.py at one of the streaming_app tests and use those as a starting point.

@codecov
Copy link

codecov bot commented Apr 7, 2021

Codecov Report

Merging #2094 (f288b34) into main (e21521f) will decrease coverage by 0.020%.
The diff coverage is 75.000%.

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #2094       +/-   ##
=============================================
- Coverage   92.146%   92.126%   -0.020%     
=============================================
  Files           38        38               
  Lines         3476      3480        +4     
  Branches       575       575               
=============================================
+ Hits          3203      3206        +3     
- Misses         185       186        +1     
  Partials        88        88               
Impacted Files Coverage Δ
sanic/response.py 94.231% <75.000%> (-0.506%) ⬇️

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 e21521f...f288b34. Read the comment docs.

@sjsadowski
Copy link
Contributor

@ajaygupta2790 Are you able to add tests for this?

@ajaygupta2790
Copy link
Contributor Author

@sjsadowski @ahopkins No, I am not able to added cases yet. I am having difficulty understanding the existing ones and make changes

@ahopkins
Copy link
Member

No worries. I can do it 😎

Thanks for the PR.

@ajaygupta2790
Copy link
Contributor Author

I was clearly using the wrong approach for this, will help in future PRs :)

@ahopkins ahopkins merged commit 9b26358 into sanic-org:main Apr 17, 2021
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.

New style streaming route handler improvements
4 participants