-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
bpo-20774: Add a JSON serializer to collections.deque #830
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
Conversation
@lisroach, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tiran, @benjaminp and @loewis to be potential reviewers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Over this looks good. Can you please add some tests.
@@ -1493,6 +1493,8 @@ encoder_listencode_obj(PyEncoderObject *s, _PyAccu *acc, | |||
/* Encode Python object obj to a JSON term */ | |||
PyObject *newobj; | |||
int rv; | |||
PyObject *mo = PyImport_ImportModule("collections"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't you need to decrease the reference counter to mo
and deque_type
after using them?
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, |
For those who arrived here like me when searching quick solution to encode dqeue object into JSON; Below code (as per documentation) shall be good enough.
|
@lisroach are you interested in writting the tests, or may I? :) |
Sorry, just realized I had this sitting for so long! @carlbordum feel free to add more tests if you think of them :) |
Lib/test/test_json/test_dump.py
Outdated
@@ -3,6 +3,7 @@ | |||
|
|||
from test.support import bigmemtest, _1G | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you could remove this blank line
Lib/test/test_json/test_dump.py
Outdated
@@ -47,7 +48,6 @@ def __lt__(self, o): | |||
d[1337] = "true.dat" | |||
self.assertEqual(self.dumps(d, sort_keys=True), '{"1337": "true.dat"}') | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this line ?
@lisroach, should this pull request be closed based on the performance benchmarks mentioned on the bug tracker or were you still working on this change? Thanks! |
This PR is stale because it has been open for 30 days with no activity. |
@lisroach, are you going to follow up this PR? |
Closing because of lack of response and conflicts, feel free to reopen if you want to work on this. |
PR for issue 20774, adding a JSON serializer to collections.deque.
https://bugs.python.org/issue20774