fix exclude_if_none to do what it should

[KeithTsui]

Great library, but two things I wanted to address in the pull request:

1. Normally wouldn't want null values to be all over the json and would prefer the field to just be absent, so changing the default for exclude_if_none=True
2. fix the exclude function to actually do a None check because I would want a boolean property to print out false.

Thanks!

[czue]

thanks for the PR! for the first change this is a pretty big change and is somewhat in conflict with how we use the library so I'm not sure we'll be willing to override the default. Will let @dannyroberts weigh in though.

The second change sounds good to me though again will defer to @dannyroberts .

FYI, the first change wreaks havoc on the tests:

======================================================================
FAIL: test_default (test.tests.JsonObjectTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/dimagi/jsonobject/test/tests.py", line 169, in test_default
    'tags': [],
AssertionError: {'doc_type': u'FamilyMember', 'first_name': u'PJ', 'br[82 chars]: {}} != {'doc_type': 'FamilyMember', 'first_name': 'PJ', 'last[124 chars]one}}
- {'base_doc': u'Person',
?              -
+ {'base_doc': 'Person',
   'brothers': [],
-  'doc_type': u'FamilyMember',
?              -
+  'doc_type': 'FamilyMember',
   'favorite_numbers': [],
-  'features': {},
+  'features': {'eyes': None, 'hair': None},
-  'first_name': u'PJ',
?                -
+  'first_name': 'PJ',
+  'last_name': None,
   'tags': []}
======================================================================
FAIL: test_dynamic_properties (test.tests.JsonObjectTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/dimagi/jsonobject/test/tests.py", line 262, in test_dynamic_properties
    'hair': None,
AssertionError: {'marmot': u'Sally', 'platypus': u'James'} != {'hair': None, 'marmot': 'Sally', 'platypus': 'James', 'eyes': None}
- {'marmot': u'Sally', 'platypus': u'James'}
+ {'eyes': None, 'hair': None, 'marmot': 'Sally', 'platypus': 'James'}
======================================================================
FAIL: test_mapping (test.tests.JsonObjectTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/dimagi/jsonobject/test/tests.py", line 208, in test_mapping
    self.assertEqual(p.to_json(), {'a': None, 'b': json_end['b']})
AssertionError: {'b': {'c': 1, 'd': u'string'}} != {'a': None, 'b': {'c': 1, 'd': 'string'}}
- {'b': {'c': 1, 'd': u'string'}}
?                     -
+ {'a': None, 'b': {'c': 1, 'd': 'string'}}
?  +++++++++++
======================================================================
FAIL: test_name (test.tests.JsonObjectTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/dimagi/jsonobject/test/tests.py", line 186, in test_name
    self.assertEqual(w.to_json(), {'_obj': None})
AssertionError: {} != {'_obj': None}
- {}
+ {'_obj': None}
======================================================================
FAIL: test_exclude_if_none (test.tests.PropertyTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/dimagi/jsonobject/test/tests.py", line 629, in test_exclude_if_none
    self.assertEqual(foo.to_json(), {'name': None})
AssertionError: {} != {'name': None}
- {}
+ {'name': None}
======================================================================
FAIL: test_typed_dict (test.tests.PropertyTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/dimagi/jsonobject/test/tests.py", line 596, in test_typed_dict
    'foo': {'hair': None, 'eyes': None},
AssertionError: {'fea[15 chars]o': {}, 'lala': {}}} != {'fea[15 chars]o': {'hair': None, 'eyes': None}, 'lala': {'ha[21 chars]ne}}}
- {'feature_map': {'foo': {}, 'lala': {}}}
+ {'feature_map': {'foo': {'eyes': None, 'hair': None},
+                  'lala': {'eyes': None, 'hair': None}}}
----------------------------------------------------------------------

[dannyroberts]

@KeithTsui thanks for the contribution. I think both of these changes have real merit, and I want to help get them in, but it's going to require a bit more than the basic implementation you've done here. As @czue noted, this library is heavily tested on exact inputs and outputs, and the changes you're suggesting are pretty far reaching. We aim heavily to be backwards compatible couchdbkit as well as previous versions of this library (jsonobject).

For both of your changes, I think there are good ways to make them backwards compatible.

For (2), I would add an additional argument called exclude_if_is_none that has the behavior you are looking for. Now, that's pretty confusing and you're right that exclude_if_none was a poor choice of words. So I'd add a deprecation warning if that param is set to True, letting the person writing the client code to use exclude_if_falsey instead. Both exclude_if_none and exclude_if_falsey will live side-by-side and do the same thing, but exclude_if_none will have a deprecation warning and exclude_if_falsey will be preferred. Then, if you want your behavior you can write:

BooleanProperty(exclude_if_is_none=True)

current code using exclude_if_none will continue to work (with a deprecation warning), and new code that wants that behavior can use

BooleanProperty(exclude_if_falsey=True)

That's for (2). For (1) I suggest having someone who wants to override the default explicitly declare that at the beginning of their code. So that would look like:

import jsonobject
jsonobject.configure_defaults(exclude_if_is_none=True)

This would have to be called before any jsonobject.JsonObject classes were declared.

Then inside JsonProperty.__init__, you could have exclude_if_is_none=None in the params and then have something like

if exclude_if_is_none is None:
    exclude_if_is_none = get_default_configuration().exclude_if_is_none

Currently, neither get_default_configuration nor configure_defaults exist, so that would have to be part of the change. Since there are plenty of default behaviors that one might want to configure, having that infrastructure in this library will make it accessible to a wider audience and I think would overall be a very healthy thing.

Please, feel free to reach out for more information. If you're interested in taking on this larger task, I'd be happy to spend the time to make sure you're able to do it well and answer questions you might have along the way.

[KeithTsui]

Hi there,

Thanks for all the prompt replies. I just found your library yesterday and
it was exactly what I needed except this one quirk. Agree with all the
comments, but one issue I can foresee with introducing two new properties
is that it might clash with the existing one. What is
exclude_if_is_none=True but exclude_if_none=False or vice versa?

I think for the second, it would be nice, but currently you guys have all
the config fields set to either True or False so introducing one that is
None would just make things awkward, right? I guess if you changed to
**kwargs that would work but that is also again a big change. I was simply
looking for a simple workaround right now for this since I don't want to be
potentially sending nulls for a lot of the optional fields in my JSON
response and having to set "exclude_if_is_none=True" for everything seemed
like a lot of boiler plate.

For now I am just doing a really bad hack in my own files:
JsonProperty.exclude = lambda self, value: value is None

Do you see that breaking anything with that? I'm happy to help with
improving the library at some point but want to make sure I'm doing the
right thing.

-Keith

On Fri, Mar 18, 2016 at 9:43 AM, Daniel Roberts [email protected]
wrote:

@KeithTsui https://github.com/KeithTsui thanks for the contribution. I
think both of these changes have real merit, and I want to help get them
in, but it's going to require a bit more than the basic implementation
you've done here. As @czue https://github.com/czue noted, this library
is heavily tested on exact inputs and outputs, and the changes you're
suggesting are pretty far reaching. We aim heavily to be backwards
compatible couchdbkit as well as previous versions of this library (
jsonobject).

For both of your changes, I think there are good ways to make them
backwards compatible.

For (2), I would add an additional argument called exclude_if_is_none
that has the behavior you are looking for. Now, that's pretty confusing and
you're right that exclude_if_none was a poor choice of words. So I'd add
a deprecation warning if that param is set to True, letting the person
writing the client code to use exclude_if_falsey instead. Both
exclude_if_none and exclude_if_falsey will live side-by-side and do the
same thing, but exclude_if_none will have a deprecation warning and
exclude_if_falsey will be preferred. Then, if you want your behavior you
can write:

BooleanProperty(exclude_if_is_none=True)

current code using exclude_if_none will continue to work (with a
deprecation warning), and new code that wants that behavior can use

BooleanProperty(exclude_if_falsey=True)

That's for (2). For (1) I suggest having someone who wants to override the
default explicitly declare that at the beginning of their code. So that
would look like:

import jsonobject
jsonobject.configure_defaults(exclude_if_is_none=True)

This would have to be called before any jsonobject.JsonObject classes
were declared.

Then inside JsonProperty.init, you could have exclude_if_is_none=None
in the params and then have something like

if exclude_if_is_none is None:
exclude_if_is_none = get_default_configuration().exclude_if_is_none

Currently, neither get_default_configuration nor configure_defaults
exist, so that would have to be part of the change. Since there are plenty
of default behaviors that one might want to configure, having that
infrastructure in this library will make it accessible to a wider audience
and I think would overall be a very healthy thing.

Please, feel free to reach out for more information. If you're interested
in taking on this larger task, I'd be happy to spend the time to make sure
you're able to do it well and answer questions you might have along the way.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#131 (comment)

[dannyroberts]

Hey @KeithTsui, I can't really comment on how well that'll work for you. Any functionality that we're committed to have working, we know works because of the test suite; without testing that, I can't say what edge cases might pop up. That said, if it works for you, go for it!

Thanks again for the suggestion. This conversation will inform changes I make in the future to make this library more accessible and its behavior more predictable. Thanks for pointing out these quirks!