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

Enable decode of timedelta pandas dtype #330

Merged
merged 1 commit into from Jan 17, 2021
Merged

Conversation

jdow7
Copy link

@jdow7 jdow7 commented Dec 14, 2020

No description provided.

@davvid
Copy link
Member

davvid commented Jan 14, 2021

Thank you so much! This looks correct, but it would be really nice if we could have a unit test to make sure that it works correctly, and to make sure that we don't break this functionality in the future.

Sorry for the delay in responding to the PRs -- I have some more time now. BTW, there is a change brewing in #338 that would be great to test in tandem with these changes. Could you maybe test that branch with these change to verify that they're all good?

Maybe we can come up with the test case together, in case you're having trouble running the tests. The tests/pandas_test.py is probably the best place to add the new tests.

Let's try to get this tested and then we'll merge this right in. Thanks again!

@davvid
Copy link
Member

davvid commented Jan 15, 2021

Ah, sorry I missed your note in #331 -- I can try to rearrange recipe you provided there into a test case as well since you mentioned you're new. Let me know if you were able to run the test suite, that's usually the first step to writing new tests since it's easier to do once it's up and running.

If I'm able to write a test that fails without these changes and then passes with them present then I'll merge this shortly thereafter. Thanks again.

davvid added a commit to davvid/jsonpickle that referenced this pull request Jan 17, 2021
Ensure that jsonpickle is able to serialize timedelta64[ns] dtypes.

Resolves jsonpickle#331
Related-to: jsonpickle#330
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/jsonpickle that referenced this pull request Jan 17, 2021
Related-to: jsonpickle#330
Signed-off-by: David Aguilar <davvid@gmail.com>
@davvid davvid merged commit 31e6eb3 into jsonpickle:master Jan 17, 2021
@davvid
Copy link
Member

davvid commented Jan 17, 2021

I added a test case to tests/pandas_test.py (near the bottom) with your recipe from #331. It worked great, thanks again!

davvid added a commit to davvid/jsonpickle that referenced this pull request Jan 31, 2021
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to davvid/jsonpickle that referenced this pull request Jan 31, 2021
* pandas:
  pandas: add support for multilevel columns
  tests/pandas_test: rework tests to leverage pytest
  pandas: code formatting
  pandas: restore timedelta support from jsonpickle#330
  Update pandas.py

Resolves jsonpickle#347
Signed-off-by: David Aguilar <davvid@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants