Strip BOM from JSON responses#145
Conversation
Codecov Report
@@ Coverage Diff @@
## master #145 +/- ##
==========================================
+ Coverage 88.01% 88.02% +0.01%
==========================================
Files 25 25
Lines 2561 2564 +3
==========================================
+ Hits 2254 2257 +3
Misses 307 307
Continue to review full report at Codecov.
|
|
|
||
| # Remove Byte Order Mark if present in string | ||
| bom = codecs.BOM_UTF8.decode(encoding='utf-8') | ||
| if data_as_str.startswith(bom): |
There was a problem hiding this comment.
A few thinkings here:
- I think in the usual workflow, we don't need it, because you patched HTTPClientResponse to utf-8-sig
- I'm hesitant on performance.
bomcan be a constant,startswithis unnecessary (lstripwill return the initial str if not startswith anyway)
I'm thinking maybe not adding this code at all, and just document it "if you give me str, you must have removed the BOM first". bytes and stream scenario will take care of the BOM.
Thoughts?
There was a problem hiding this comment.
Changed as requested. I believe we have 3 options:
- (BEST) We keep this as is (not best performance) for now and then we can create a PR for requests library. I say this because when debugging I saw that the encoding was
utf-8and apparent encoding from chardet wasutf-8-sig. So we can just specify to give preference apparent encoding. - We try to create a global boolean flag within msrest to remove BOM only if set to
true. - Just documented it. This option would not help me. I was able to create a SDK directly from swagger file from my fork, but if i use the master i need to forcefully set
response.encoding = 'utf-8-sig'.
There was a problem hiding this comment.
The PR for requests has been created and, if they merge it there, I believe all the changes here will not be required at all. :)
|
Hi @eduardomourar ! |
|
|
||
| _LOGGER = logging.getLogger(__name__) | ||
|
|
||
| BOM = codecs.BOM_UTF8.decode(encoding='utf-8') |
There was a problem hiding this comment.
_BOM please, I like to keep my stuff private :)
There was a problem hiding this comment.
Last tiny detail and I merge it ;)
There was a problem hiding this comment.
you are completely right!
|
Thanks @eduardomourar again for the contribution! |
3 participants
@lmazuel and @susam, this fixes issue #144.