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

Accept an errors parameter for text decoding ClientResponses #1542

Merged
merged 5 commits into from
Jan 21, 2017

Conversation

bmuller
Copy link
Contributor

@bmuller bmuller commented Jan 16, 2017

What do these changes do?

Users can now give a parameter specifying what should happen to decoding errors when asking for the text() from a ClientResponse. This is passed directly into the bytes() decode method.

The internet is full of websites that have incorrectly specified their character encoding or that have one or two characters on a page that are not encoded correctly. Right now, the aiohttp library just throws an exception when this happens - for a more resilient solution, users should be able to optionally provide an alternative error handling strategy for incorrectly encoded characters.

Are there changes in behavior for the user?

Nope - just a new optional parameter.

Related issue number

Nope.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • Add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new entry to CHANGES.rst
    • Choose any open position to avoid merge conflicts with other PRs.
    • Add a link to the issue you are fixing (if any) using #issue_number format at the end of changelog message. Use Pull Request number if there are no issues for PR or PR covers the issue only partially.

@bmuller bmuller changed the title Accept an errors parameter for text encoding ClientResponses Accept an errors parameter for text decoding ClientResponses Jan 16, 2017
@codecov-io
Copy link

codecov-io commented Jan 16, 2017

Current coverage is 98.91% (diff: 100%)

Merging #1542 into master will decrease coverage by 0.01%

@@             master      #1542   diff @@
==========================================
  Files            30         30          
  Lines          6995       6989     -6   
  Methods           0          0          
  Messages          0          0          
  Branches       1169       1162     -7   
==========================================
- Hits           6920       6913     -7   
  Misses           37         37          
- Partials         38         39     +1   

Powered by Codecov. Last update 275e3b7...1a4ae3c

@fafhrd91
Copy link
Member

Could you add test for this change?

@bmuller
Copy link
Contributor Author

bmuller commented Jan 21, 2017

@fafhrd91 sure - what would you like to be tested though? This is a pretty inconsequential change that just passes another optional parameter down to Python std lib call.

@fafhrd91
Copy link
Member

test that errors param get passed to decode function, also could you use errors keyword argument for decode function.

@bmuller bmuller force-pushed the text-encoding-error-handling branch from 1a4ae3c to 9c7f964 Compare January 21, 2017 21:39
@bmuller
Copy link
Contributor Author

bmuller commented Jan 21, 2017

OK - let me know if there's anything else you need @fafhrd91

@fafhrd91
Copy link
Member

great! thanks

@fafhrd91 fafhrd91 merged commit 5508bab into aio-libs:master Jan 21, 2017
@bmuller bmuller deleted the text-encoding-error-handling branch January 21, 2017 21:51
@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants