search

avian2 / jsonmerge

Merge a series of JSON documents.
MIT License
214 stars 25 forks source link

Provide support for alternative object classes #27

Closed RayPlante closed 7 years ago

RayPlante commented 7 years ago

The main motivation for this PR is to provide a way to have jsonmerge to use the OrderedDict class for JSON object data in the merged result. This is analogous to the json package's support for the object_pairs_hook parameter. And like that json parameter, this PR tries to generalize the ability to customize the JSON object class.

The changes in the interfaces occur in the Merger class and the ObjectMerge strategy class. The Merger class has built-in options for using either dict or OrderedDict, so the easiest way to use the latter throughout the merged result is to set the constructor's objclass parameter to 'OrderedDict'.

This PR also allows one to set the object class on a per JSON object type level. This is done via the objclass mergeOption to the objectMerge strategy. Setting this option can be set to either 'OrderedDict' or 'default' to have ObjectMerge or dict used as an object container for that type. Other classes can be made available to this option by providing a dictionary of class via the Merger constructor's obj_cls_menu.

For full details, see the new text added to the README.rst and inline documentation for the Merger and ObjectMerge class. This PR includes some added tests.

RayPlante commented 7 years ago

I would like to chase down the Travis failures for the different python versions, but nothing is showing in the output logs. Is this normal?

RayPlante commented 7 years ago

(I think there was an issue with Travis preventing me from seeing logs; it's okay now.)

Anyway, I've solved the python3 and python2.6 failures as you can see. OrderedDict, of course, is not available in python 2.6.

avian2 commented 7 years ago

Hi. Thanks for your pull request. I'm a bit busy at the moment and it will take me a few days to look into it.

avian2 commented 7 years ago

Thanks again for a very nice pull request and sorry for the late reply. I'm really happy to see such a well documented change.

Before merging this, I would prefer to rename a few things for consistency. Names in JSON Schema use camelCase, so I would rather use objClass name for the option. Also, obj_cls_menu would be better named objclass_menu to use the same contraction as in other places.

I already did these changes in this branch: https://github.com/avian2/jsonmerge/tree/objclass

Another thing is that default name for using the dict class seems inconsistent. I think it would be better to simply use dict in obj_cls_menu.

avian2 commented 7 years ago

I reworked a bit how the default class is handled. It's now possible the specify dict and OrderedDict directly in objClass. Default value is stored under _default key in objclass_menu and is not part of the public interface.

This is the version that I plan to merge: https://github.com/avian2/jsonmerge/tree/objclass

RayPlante commented 7 years ago

This all looks great--thanks for the clean up! I'll give it a test drive.

RayPlante commented 7 years ago

I took the objclass branch for a quick once-around. Apart from the minor Merger constructor argument change, this appears to be a drop-in replacement for my submitted version; everything works fine. I'm developing some code around this at the moment, so if I run into any bugs or issues, I'll certainly pass them on. Thanks for the great code! It's a perfect match to what I was looking for.

avian2 commented 7 years ago

Merged. Thanks for your contribution.