Fix NaN bug in ujson #46628
Fix NaN bug in ujson #46628
Conversation
|
cc @WillAyd |
| @@ -417,6 +417,9 @@ def test_encode_time_conversion_dateutil(self): | |||
| def test_encode_as_null(self, decoded_input): | |||
| assert ujson.encode(decoded_input) == "null", "Expected null" | |||
|
|
|||
| def test_decode_nan(self, decoded_input): | |||
There was a problem hiding this comment.
Can you create a test in one of the other modules in this same directory? I think this module was copied directly from ujson but is detached from what an end user would see, as no end user really imports ujson through pandas.
Would be a better test if we can re-create how an end user would come to this through one of the pandas objects
There was a problem hiding this comment.
I've added a test in pandas.tests.io.json.test_pandas that uses the MWE from the bug report. I think this test should stay here as well, as a similar test now exists in ujson itself.
There was a problem hiding this comment.
Alternatively, we could consider "re-vendoring" ujson now that this bug is fixed in it's main branch as well. But I'm not sure if pandas has other modifications or not (and if their are, they probably should be backported anyway).
rhshadrach
added
Bug
Missing-data
|
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
This should still be merged and needs review. |
|
Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen. |
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.This ports the fixes I made in ujson ultrajson/ultrajson#514 wrt reading NaN values to pandas.