Skip to content

OrderedDict compatible#11

Open
innov8ian wants to merge 4 commits intoboris317:masterfrom
rebottle:master
Open

OrderedDict compatible#11
innov8ian wants to merge 4 commits intoboris317:masterfrom
rebottle:master

Conversation

@innov8ian
Copy link

It seems there are 3 changes here.
Firstly, my editor (atom) likes to delete training whitespace at the end of lines. Annoying and distracting, since it creates a lot of lines with changes that are not really changes. My diff engine did not even highlight these lines as changed.

Secondly, I moved to relative imports as this give flexibility where the package is in the tree.

Thirdly, and most significantly, I added code to allow working with 'OrderedDict' which allow maintaining the order of key value pairs. So dictionaries which are not classes can be returned as OrderedDicts when passing method to 'object_hook_pair'. Previously any use of 'object_hook_pairs' blocked object decoding. I modified the code to still work with object hook_pairs.

Hope you like the addition.

Allow for 'object_pairs_hook' to be in call, sense this and adjust automatically.  Previously object_pairs_hook would disable object decoding for __type__ objects completely.
@boris317
Copy link
Owner

boris317 commented Feb 6, 2016

Moving to relative imports is fine but we should do it for ALL of the files. I've glanced at the code and I'm not entirely sure what baseHooks is supposed to do. Please add some unit tests to exercise this new code.

@innov8ian
Copy link
Author

This was a first communication about the JsonWeb. Before being thorough I was waiting for feedback.

Since I take it you are happy with moving to relative imports, as you suggest I will look to complete that task.

The 'base_hook' is to allow for code which already has a use case for either 'object_hook' or 'object_pairs_hook' and one of these is already present in the call to be changed to use this library in place of the standard json library.

The result is that the previous 'object_hook' (or 'base_hook') drops in priority below the JsonWeb objects. So once JsonWeb library decides the object is not to be processed within this library, then whatever object hook was previously used with then be called.

My own case was 'OrderedDict()' being called using 'object_pairs_hook'.

decode.loader(jsonString ,object_pairs_hook=OrderedDict)

will now perform decode objects with type as before, but will produce OrderedDict objects for those not converted into an object by JsonWeb. None that the original object_pairs_hook ranks in priority below JsonWeb and is only called after JsonWeb code determines this object should not be processed by JsonWeb.

Before the change, the result of the call above is to disable JsonWeb decoding objects completely and leave all as OrderedDicts.
Hope this helps.

If you agree with the concept, which leaves unchanged where there was no previous 'object_hook' or 'object_pairs_hook' parameter, but produces a useful result in cases where they are present, then I will look to add unit tests.

I have done a quick update for the two style points you noted, but time constraints mean I will be slower on the other points.

@innov8ian
Copy link
Author

I have been tardy in converting my original py.test tests to nose, partly because I have also been looking at even more additions.

I have come to the conclusion that the reason I am looking at some changes is because I am using the module in a different manner than the original intent. The question then becomes, move to an entirely different library for the different purpose, or extend the original to attempt both goals.

I see the original intent as being to serialise objects to JSON for the purpose of then retrieving those objects. However my intent can be better described as using objects to build json messages to be sent, and objects to process json messages received.

One key difference this goal brings is that order can be significant in building messages to send or processing message received. Hence a need to work in OrderedDicts as opposed to dict types.
But there are other differences.

This different goal is best served with a few significant additions, and for this different goal significant parts of the original solution are redundant and in some cases require workarounds or ways to bypass what are in the original use case significant features.

In the end, a key question relates to the attitude of the the original package developer to accommodating a quite different set of goals.

Feedback appreciated.

@boris317
Copy link
Owner

boris317 commented Mar 4, 2016

I'm still not sure how exactly you are using JsonWeb? Which parts of it are you using and how?

@innov8ian
Copy link
Author

Sorry about slow response. But I have been giving consideration for the answer.

As far as I can see, the intent of my use and json web is at variance.

The tagline for JSONweb is:
Add JSON (de)serialization to your python objects

The tagline for my use would be:
generate and process you JSON messages from objects rather than dictionaries.

The impact is that for my use it is necessary to be able to produce required json messages, where as for JSON web original use the requirement is that json can represent and restore objects. One key implication is that for my use case, order of json output is important, where as for the original use case it is not.

Also the clever init() function scanning for parameters seems to only get in the way for my use case.

One possibility is I derive a new implementation of what is ideal for my use case, that although inspired by use of JSON web would revisit the original philosophies. Once that is complete, it could be reviewed to decide what, if any, is relevant to the original JSONweb.

My greatest reluctance to commit to a plan is I simply have not yet explored the JSONweb schema and validators functionality yet. Might be key features there I am not seeing yet.

@innov8ian innov8ian closed this Mar 10, 2016
@innov8ian innov8ian reopened this Mar 10, 2016
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.

2 participants