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.

Sign up for GitHub

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

Jump to bottom

Fix encoding error with non-prettified encoded responses #1168

Merged
merged 19 commits into from
Oct 6, 2021
Merged

Fix encoding error with non-prettified encoded responses #1168

merged 19 commits into from
Oct 6, 2021

Conversation

BoboTiG
Copy link
Contributor

@BoboTiG BoboTiG commented Sep 30, 2021 •
edited
Loading

Proposal

  1. Move response Content-Type checks from the PrettyStream class to the EncodedStream parent class.
  2. Remove --format-option response.as and --response-as options.
  3. Introduce --response-encoding and --response-mime options.

Fixes #1167.

@BoboTiG
Fix encoding error with non-prettified encoded responses
0b5f4d6 
Removed `--format-option response.as` an promote `--response-as`: using
the format option would be misleading as it is now also used by non-prettified
responses.
@codecov-commenter
Copy link

codecov-commenter commented Sep 30, 2021 •
edited
Loading

Codecov Report

Merging #1168 (115aef9) into master (4d7d6b6) will decrease coverage by 0.61%.
The diff coverage is 91.56%.

❗ Current head 115aef9 differs from pull request most recent head 6cbe2b2. Consider uploading reports for the commit 6cbe2b2 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1168      +/-   ##
==========================================
- Coverage   97.28%   96.66%   -0.62%     
==========================================
  Files          67       70       +3     
  Lines        4235     4471     +236     
==========================================
+ Hits         4120     4322     +202     
- Misses        115      149      +34
Impacted Files Coverage Δ
tests/test_binary.py 100.00% <ø> (ø)
httpie/compat.py 16.66% <9.09%> (-83.34%) ⬇️
tests/conftest.py 75.75% <50.00%> (-11.20%) ⬇️
tests/test_ssl.py 89.88% <63.63%> (-5.05%) ⬇️
httpie/models.py 95.31% <72.72%> (-2.06%) ⬇️
httpie/output/formatters/colors.py 92.66% <83.33%> (-0.92%) ⬇️
httpie/__init__.py 100.00% <100.00%> (ø)
httpie/cli/argparser.py 95.56% <100.00%> (-0.80%) ⬇️
httpie/cli/argtypes.py 95.27% <100.00%> (+0.36%) ⬆️
httpie/cli/definition.py 100.00% <100.00%> (ø)
... and 29 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 71adcd9...6cbe2b2. Read the comment docs.

jkbrzt added 2 commits October 4, 2021 20:43
@jkbrzt
Encoding refactoring
491188d 
* split --response-as into --response-mime and --response-charset
* add support for Content-Type charset for requests printed to terminal
* add support charset detection for requests printed to terminal without a Content-Type charset
* etc.
@jkbrzt
test_unicode.py → test_encoding.py
252fe02
Ousret
Ousret reviewed Oct 4, 2021
View reviewed changes
httpie/encoding.py Outdated
>>> too_short = ']"foo"'
>>> detected = from_bytes(too_short.encode()).best().encoding
>>> detected
'utf_16_be'

This comment was marked as spam.

Sign in to view

Copy link
Member

Choose a reason for hiding this comment

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

Good to know! What has changed?

This comment was marked as spam.

Sign in to view

Copy link
Member

@jkbrzt jkbrzt Oct 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

That’s exactly what we needed. Is there still some length threshold below which it’s unreasonable to rely on the detected encoding? Or a way to get some sort of confidence interval for the best match?

Copy link
Contributor Author

@BoboTiG BoboTiG Oct 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe could we require charset_normalizer>=2.0.5 and drop our own TOO_SMALL_SEQUENCE check?

I am not sure that charset_normalizer >= 2.0.5 is available on all OSes for our package thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I committed d52a483 just to see how it goes.

This comment was marked as spam.

Sign in to view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll revert d52a483 as it seems too new.

@BoboTiG BoboTiG requested a review from jkbrzt October 5, 2021 12:10
@BoboTiG BoboTiG merged commit 4f1c944 into master Oct 6, 2021
@BoboTiG BoboTiG deleted the mickael/oss-73-fix-encoding-error-with-non-prettified branch October 6, 2021 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ousret Ousret Ousret left review comments

@jkbrzt jkbrzt Awaiting requested review from jkbrzt

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Encoding error with non-prettified encoded responses
4 participants
@BoboTiG @codecov-commenter @jkbrzt @Ousret