Updated JSON serialization of Date and CalendarDate to support None

[jlstevens]

Simple fix that addresses #578 issue for None valued Date parameters and does the same for CalendarDate parameters.

[jlstevens]

@jbednar @philippjfr Ready for review assuming the tests go green on CI (they work fine locally).

[codecov-commenter]

Codecov Report

Merging #582 (a3e5777) into master (74f7ac2) will increase coverage by 0.18%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #582      +/-   ##
==========================================
+ Coverage   82.23%   82.41%   +0.18%     
==========================================
  Files           4        4              
  Lines        2994     3014      +20     
==========================================
+ Hits         2462     2484      +22     
+ Misses        532      530       -2     

Impacted Files	Coverage Δ
numbergen/__init__.py	80.32% <0.00%> (+0.40%)	⬆️
param/serializer.py	86.95% <0.00%> (+0.86%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74f7ac2...a3e5777. Read the comment docs.

[jlstevens]

Tests have passed except for the pypy suite due to an unrelated issue.

[jbednar]

Thanks! The code looks good. Some questions:

• Thanks for realizing that CalendarDate needed the fix too. It looks like there is a lot of overlap between Date and Calendar date, with nearly identical changes needed in each. I think users will expect those classes to behave the same in all cases apart from the type, so for futureproofing we could consider refactoring them into a BaseDate class with a strformat parameter ("%Y-%m-%d" vs. "%Y-%m-%dT%H:%M:%S.%f"), or just making Date be that baseclass and having very little code in CalendarDate so that those two classes always stay in sync.
• Can you scan for any other classes that allow_None that also need this fix?

[jlstevens]

Thanks for prompting me to have another look!

I realize that my initial impression was correct and JSONNullable is applied even for the special schema methods. I'll push a commit now to revert that unnecessary change.

Otherwise it is about checking how the serialize and deserialize handle 'null'. The full list of parameters using these methods are: Tuple, Array, DataFrame, Date (done) and CalendarDate (done).

I'll make sure Tuple, Array, DataFrame work properly now.

[jlstevens]

I think users will expect those classes to behave the same in all cases apart from the type, so for futureproofing we could consider refactoring them into a BaseDate class with a strformat parameter ("%Y-%m-%d" vs. "%Y-%m-%dT%H:%M:%S.%f"), or just making Date be that baseclass and having very little code in CalendarDate so that those two classes always stay in sync.

At a glance that does look to be true but looking closer there isn't that much shared. The isinstance checks are different and other than the strformat format, CalendarDate.deserialize uses the .date() method. The serialize methods are also different so on balance I don't think there is that strong an incentive to make a base class.

[jbednar]

Ah, didn't spot ".date()". Ok, keeping them separate is fine. Having a general declarative implementation of the instance checks across many Parameter types would be a separate task, but would seem to simplify things.

[jlstevens]

@philippjfr @jbednar Any idea what the rationale might have been to hardcode allow_None=True for Array? Is there a reason arrays always have to accept None?

class Array(ClassSelector):
    """
    Parameter whose value is a numpy array.
    """

    def __init__(self, default=None, **params):
        from numpy import ndarray
        super(Array, self).__init__(ndarray, allow_None=True, default=default, **params)

[jbednar]

I would assume that's because Numpy is not a required dependency and thus we can't have an array as the default value. That said, I think the right thing to do is to have the default be None but not specify allow_None, so that when someone instantiates their own Array parameter they can put their own default in, typically non-None, and then separately decide whether to allow None. That's a breaking change, but I think it's the right thing to do, as I don't think allow_None should have been hardcoded except when using the default None value.

[jlstevens]

Your suggestion about allow_None makes sense to me and should be put into another issue.

Otherwise, I have added None/'null' support for Tuple, Array and DataFrame. Ready for review/merge once more.

[jbednar]

Looks good, but please consistently choose either an early return style:

if value is None:
    return 'null'

return val...

or an if/else:

if value is None:
    return 'null'
else:
    return val...

Right now it's a mix of the two, and thus less easy to scan.