-
-
Notifications
You must be signed in to change notification settings - Fork 74
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
Allow (de)serializing CalendarRange and DateRange Parameters #625
Conversation
Codecov Report
@@ Coverage Diff @@
## master #625 +/- ##
==========================================
+ Coverage 82.81% 82.89% +0.08%
==========================================
Files 5 5
Lines 3037 3070 +33
==========================================
+ Hits 2515 2545 +30
- Misses 522 525 +3
Continue to review full report at Codecov.
|
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.
Looks good to me!
Actually any container Parameter that holds a datetime object won't be able to be serialized due to the limitations of the basic Python JSON encoder. As such the changes made in this PR aren't yet enough to fix the issue I observed in Lumen, which uses the The way forward may be to subclass |
JSON has no datetime representation so it's actually not possible to directly decode a Python datetime value encoded to a JSON string. Seems like a limitation that should be mentioned in the documentation. The Lumen/Panel issue needs to be fixed separately. |
This seems sufficient for the specific bug we are fixing here. In the longer run it would be nice to work out some more general solution but I suspect we'd need composite parameter types to make that feasible. |
I got the following error while working on a Lumen app that has a DateRange filter widget synced to the URL:
We found out that the
DateRange
andCalendarRange
parameters inherit from theTuple
parameter (de)serialization methods, while they should override these methods as Python'sjson
module can't serialize datetime objects (import datetime as dt, json; json.dumps(dt.datetime.now())
raises the TypeError mentioned above).This PR adds these methods to the two parameters, and test them. I've added tests, including tests that use
np.datetime64
andpd.Timestamp
objects since they appeared to be supported (I learnt thatpd.Timestamp
objects inherits fromdatetime.datetime
).