bpo-30193: Allow to load buffer objects with json.loads() #1334
Conversation
|
@fafhrd91, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tiran, @benjaminp and @ezio-melotti to be potential reviewers. |
| if bstartswith((codecs.BOM_UTF16_BE, codecs.BOM_UTF16_LE)): | ||
| return 'utf-16' | ||
| if bstartswith(codecs.BOM_UTF8): | ||
| for prefix in (codecs.BOM_UTF32_BE, codecs.BOM_UTF32_LE): |
There was a problem hiding this comment.
Why was this change made?
There was a problem hiding this comment.
memoryview does not provide "startswith" function
There was a problem hiding this comment.
D'oh, completely missed that, it's late here :) Anyway, support for bytes was added in https://bugs.python.org/issue17909 by Serhiy so it might make sense to ping him.
Lib/json/__init__.py
Outdated
| for prefix in (codecs.BOM_UTF16_BE, codecs.BOM_UTF16_LE): | ||
| if b[0:len(prefix)] == prefix: | ||
| return 'utf-16' | ||
| if b[0:len(codecs.BOM_UTF8)] == codecs.BOM_UTF8: |
There was a problem hiding this comment.
Replace [0:len(...)] with [:len(...)].
Lib/test/test_json/test_decode.py
Outdated
| @@ -20,6 +21,15 @@ def test_empty_objects(self): | |||
| self.assertEqual(self.loads('[]'), []) | |||
| self.assertEqual(self.loads('""'), "") | |||
|
|
|||
| def test_memoryview(self): | |||
| data = b'{"key": "val"}' | |||
There was a problem hiding this comment.
Please create the memoryview() on that line to make the test more explicit.
Misc/NEWS
Outdated
| @@ -102,6 +102,8 @@ Core and Builtins | |||
|
|
|||
| - bpo-29546: Improve from-import error message with location | |||
|
|
|||
| - bpo-30193: Allow to load buffer objects with ``json.loads()`` | |||
There was a problem hiding this comment.
The NEWS entry should be move at the top the Library section.
|
@Haypo updated according comments |
| return 'utf-16' | ||
| if bstartswith(codecs.BOM_UTF8): | ||
| for prefix in (codecs.BOM_UTF32_BE, codecs.BOM_UTF32_LE): | ||
| if b[:len(prefix)] == prefix: |
There was a problem hiding this comment.
This doesn't work for e.g. memoryview(array.array('u', '\ufeff[1,2,3]')).
There was a problem hiding this comment.
purpose of this change is to allow to use c-ext classes that supports buffer protocol.
I can remove memoryview from allowed type, and if encoding detection fails then fail with TypeError.
proper solution would be to write memoryview like object that supports only "b|B" format
There was a problem hiding this comment.
any conclusion on this ticket?
There was a problem hiding this comment.
Is it reasonable to require unicode arrays for this patch? They are deprecated after all.
There was a problem hiding this comment.
I suggest to start simple and raise a TypeError if it's memory view with view.format != 'B'.
|
To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request. If/when the requested changes have been made, please leave a comment that says, |
|
It looks like all comments were addressed or the OP was seeking further feedback from core on the requested changes. Would it be possible for someone from core to review this again? |
|
@jakirkham Revived in #14977 |
| @@ -317,6 +317,8 @@ Extension Modules | |||
| Library | |||
| ------- | |||
|
|
|||
| - bpo-30193: Allow to load buffer objects with ``json.loads()`` | |||
There was a problem hiding this comment.
You should add a NEWS entry using the blurb tool, and revert this change.
|
As this is from an unknown repository, I'm going to close this PR. A replacement PR has already been created. |
it is not possible to load buffer object with json.loads()