search

avian2 / jsonmerge

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

allowing sorting as an option #51

Closed pmvieira closed 2 years ago

pmvieira commented 3 years ago

This PR adds an option to sort complex arrays based on a key when the merge strategy is either append or arrayMergeById

coveralls commented 3 years ago

Coverage Status

Coverage increased (+0.02%) to 97.481% when pulling deb62662d6788b4afed3f03d882e6737676e3a92 on pmvieira:feature/allowing-sorting-as-an-option into 8ce3beb01010b64c3bf18a4798e67334b8d72e79 on avian2:master.

coveralls commented 3 years ago

Coverage Status

Coverage increased (+0.02%) to 97.481% when pulling deb62662d6788b4afed3f03d882e6737676e3a92 on pmvieira:feature/allowing-sorting-as-an-option into 8ce3beb01010b64c3bf18a4798e67334b8d72e79 on avian2:master.

avian2 commented 3 years ago

Thanks for the pull request!

My concern is that i[sortBy] won't work with array elements that are anything other than JSON objects. Can you make the sortBy work by resolving a JSON pointer fragment? See how idRef argument is implemented using resolve_fragment. This way the sorting could work with any type.

pmvieira commented 3 years ago

You're welcome 😄 I've been mulling about what you said and it makes perfect sense, however it is important that that more broad use case is nailed correctly so that we don't miss something important. Since we are documenting that this option that we are introducing is meant for complex structures, how you would feel about moving forward as we are now, and then iterate over this in order to nail the other use cases spot-on?

avian2 commented 3 years ago

The problem I have with going with this simple solution and then later moving to JSON pointers is that it would be a breaking change (for example, the meaning of sortBy="/foo" would change). JSON pointers are used throughout JSON schema standard for referring to parts of the document and the syntax is already used in idRef. I really don't see much reason not to go with it here as well.

avian2 commented 2 years ago

I'm closing this pull request now. I've addressed the issues I mentioned in my previous comments myself by building on top of your commit. It's merged into the master branch now.

I dropped your commit that changed the jsonmerge version number, which is why GitHub doesn't consider this a merged pull request.

The latest code now supports sortByRef and sortReverse options for the array strategies. Those implement the sorting feature using JSON pointers.