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

Handle case when headers argument of ResponseStream constructor is None #2729

Merged
merged 5 commits into from
Jul 9, 2023

Conversation

liamcoatman
Copy link
Contributor

The default value for the headers argument is None. This leads to an Exception when headers is passed to the Header constructor, which is expecting a Mapping type.

@liamcoatman liamcoatman requested a review from a team as a code owner March 30, 2023 08:46
@liamcoatman liamcoatman changed the title Handle case when headers is None Handle case when headers argument of ResponseStream constructor is None Mar 30, 2023
@codecov
Copy link

codecov bot commented Mar 30, 2023

Codecov Report

Patch coverage: 66.666% and project coverage change: -0.098 ⚠️

Comparison is base (c21999a) 88.981% compared to head (6a47eb0) 88.884%.

Additional details and impacted files
@@              Coverage Diff              @@
##              main     #2729       +/-   ##
=============================================
- Coverage   88.981%   88.884%   -0.098%     
=============================================
  Files           92        92               
  Lines         6979      6981        +2     
  Branches      1190      1191        +1     
=============================================
- Hits          6210      6205        -5     
- Misses         527       531        +4     
- Partials       242       245        +3     
Impacted Files Coverage Δ
sanic/response/types.py 85.082% <66.666%> (+0.166%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

Can you add some unit tests?

@liamcoatman
Copy link
Contributor Author

@ahopkins I've added a test, which was failing prior to my fix and is now passing. I've probably put it in the wrong place, so let me know if you want me to move it.

@ahopkins
Copy link
Member

It's a long file, but I'd dump it here: https://github.com/sanic-org/sanic/blob/main/tests/test_response.py

@liamcoatman
Copy link
Contributor Author

In case you missed it, I moved the test to test_response.py as suggested

@QylinFly
Copy link

ResponseStream is not stream,bug?

@ahopkins
Copy link
Member

ResponseStream is not stream,bug?

Sorry, I don't understand your question.

@ahopkins ahopkins merged commit 11a0b15 into sanic-org:main Jul 9, 2023
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.

4 participants