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

Automatically enable --stream on server sent events #1226

Merged
merged 4 commits into from
Dec 8, 2021

Conversation

isidentical
Copy link
Contributor

Resolves #376.

@codecov-commenter
Copy link

codecov-commenter commented Dec 2, 2021

Codecov Report

Merging #1226 (55f4c2c) into master (4d7d6b6) will decrease coverage by 0.55%.
The diff coverage is 95.05%.

❗ Current head 55f4c2c differs from pull request most recent head fd8897e. Consider uploading reports for the commit fd8897e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1226      +/-   ##
==========================================
- Coverage   97.28%   96.72%   -0.56%     
==========================================
  Files          67       81      +14     
  Lines        4235     5409    +1174     
==========================================
+ Hits         4120     5232    +1112     
- Misses        115      177      +62     
Impacted Files Coverage Δ
tests/test_binary.py 100.00% <ø> (ø)
httpie/compat.py 31.11% <27.90%> (-68.89%) ⬇️
tests/conftest.py 77.14% <58.33%> (-9.82%) ⬇️
tests/test_ssl.py 91.01% <66.66%> (-3.93%) ⬇️
httpie/manager/__main__.py 82.35% <82.35%> (ø)
httpie/models.py 94.73% <83.33%> (-2.64%) ⬇️
httpie/output/formatters/colors.py 92.66% <88.88%> (-0.92%) ⬇️
httpie/manager/core.py 92.85% <92.85%> (ø)
httpie/manager/plugins.py 93.39% <93.39%> (ø)
httpie/plugins/manager.py 96.87% <95.00%> (+4.98%) ⬆️
... and 57 more

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 8fe1f08...fd8897e. Read the comment docs.

@@ -49,7 +49,8 @@ def test_chunked_stdin(httpbin_with_chunked_support):
)
assert HTTP_OK in r
assert 'Transfer-Encoding: chunked' in r
assert r.count(FILE_CONTENT) == 2
# One original (request) + one encoded (httpbin's response, ascii)
assert r.count(FILE_CONTENT) + r.count(RAW_FILE_CONTENT) == 2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for these changes is that, httpbin mirrors the headers when it is responding. Since it is also mirroring the Transfer-Encoding, we treat this response as if it was chunked and use different delimiting techniques. So because of that, we stop formatting it as JSON, and not decode the actual unicode symbols (since that happens on formatting). Which I think is a plausible behavior, considering in this case, the server is the wrong side.

@isidentical
Copy link
Contributor Author

Tests are failing on 3.7/3.6, probably due to some mocking behavior changes. Will look at them on the morning.

@jkbrzt
Copy link
Member

jkbrzt commented Dec 4, 2021

Why is it enabled for all chunked responses? This really only makes sense for server-sent events, where each line can be considered a body. https://httpie.io/docs#disabling-buffering

@isidentical
Copy link
Contributor Author

Why is it enabled for all chunked responses? This really only makes sense for server-sent events, where each line can be considered a body.

Ah, alright! Will do that then, thanks for the correction. I thought you initially meant enabling this for all chunked responses, since the quote in this implied that. Will fix it!

@isidentical isidentical changed the title Automatically enable --stream when used chunked encoding Automatically enable --stream on server sent events Dec 7, 2021
@jkbrzt jkbrzt merged commit 207b970 into httpie:master Dec 8, 2021
@mivade
Copy link

mivade commented Dec 8, 2021

Woah, this is great! I'm not using SSE as much as I was when I filed #376 but this is really nice to see.

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.

Server-sent events explicitly requires --stream
4 participants