Skip to content
This repository has been archived by the owner on Jul 30, 2018. It is now read-only.

Comparison API #233

Merged
merged 6 commits into from
Aug 25, 2017
Merged

Comparison API #233

merged 6 commits into from
Aug 25, 2017

Conversation

kitsonk
Copy link
Member

@kitsonk kitsonk commented Nov 7, 2016

This PR implements a comparison API as part of core.

Fixes #138

The Comparison API can deal with Arrays and "plain" Objects. Plain objects are objects that have either Object as a constructor (or no constructor) and the properties values of the object are either primitives, arrays, or plain objects. For objects, only enumerable own properties are considered.

The module compare exports two functions diff and patch.

diff() returns the difference between the first and the second argument and describes the steps to take to make the first object look like the second object.

patch() takes a target and applies a set of changes which will change the target.

@jsf-clabot
Copy link

jsf-clabot commented Nov 7, 2016

CLA assistant check
All committers have signed the CLA.

@dylans dylans added this to the 2016.11 milestone Nov 9, 2016
@kitsonk
Copy link
Member Author

kitsonk commented Nov 15, 2016

@maier49 @rorticus @vansimke could you please review

@agubler
Copy link
Member

agubler commented Dec 5, 2016

@maier49 can you give this a review please?

@dylans dylans modified the milestones: 2016.11, 2016.12 Dec 5, 2016
Copy link
Contributor

@maier49 maier49 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main concern here is how this fits in with the existing comparison API provided in Stores. Stores uses a patch implementation that serializes to JSON Patch.

I think the checks for plain objects and some other things in here are a little better than how Stores is currently handling it, but I don't know if we should replace the diff functionality without answering the question of how we want to serialize patches for sending to the server. If we do want to use JSON Patch, then it seems odd to switch to a patch/diff implementation that doesn't support that from one that does.

@dylans dylans modified the milestones: 2017.01, 2016.12 Dec 20, 2016
@dylans
Copy link
Member

dylans commented Jan 31, 2017

We should probably decide what to do with this... do we need to leverage more of what dojo/stores has instead, etc.? @maier49 @agubler

@dylans dylans modified the milestones: 2017.01, 2017.02 Jan 31, 2017
@codecov-io
Copy link

codecov-io commented Feb 2, 2017

Codecov Report

Merging #233 into master will increase coverage by 0.39%.
The diff coverage is 99.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #233      +/-   ##
==========================================
+ Coverage   93.88%   94.28%   +0.39%     
==========================================
  Files          39       40       +1     
  Lines        2144     2309     +165     
  Branches      408      463      +55     
==========================================
+ Hits         2013     2177     +164     
  Misses         52       52              
- Partials       79       80       +1
Impacted Files Coverage Δ
src/compare.ts 99.39% <99.39%> (ø)

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 a77d9e7...aa0e124. Read the comment docs.

@maier49
Copy link
Contributor

maier49 commented Feb 2, 2017

The patch implementation in stores could pretty easily be moved over to core. It only relies on a few utility functions outside of the patch package. Since it already supports serialization to JSON patch and there's a PR open to support JSON Merge Patch I'm still inclined to think that it would be better to move it over to core than to have competing implementations or switch to an implementation that doesn't yet have a plan for serialization.

We could address any issues with the API in the process of moving it over.

@dylans
Copy link
Member

dylans commented Feb 2, 2017

@maier49 ok, let's do that!

@agubler
Copy link
Member

agubler commented Feb 17, 2017

So shall we close this pending @maier49's work to move the implementation over from stores?

@dylans dylans modified the milestones: 2017.02, 2017.03 Mar 1, 2017
@dylans dylans modified the milestones: 2017.03, 2017.04 Apr 2, 2017
@kitsonk
Copy link
Member Author

kitsonk commented Apr 6, 2017

I have refreshed this PR from what I have had to do in intern-helper which needed to use this API.

Also, my understanding is that @pottedmeat had a chat with @maier49 about the usage in stores for dgrid and they agreed that it might be better to take what is here and iterate on it to make it work for stores. If that is still the case, we should land this and figure out if there are any features that are needed for stores.

Also, widget-core uses a diff function for properties, which we should evaluate if this API is sufficient for that, or if there are other changes required.

@maier49
Copy link
Contributor

maier49 commented Apr 6, 2017

@kitsonk the main thing for stores is just providing an appropriately serialized version of a diff for a PATCH request. I'm ok with stores using its own implementation until we figure that out here.

@kitsonk
Copy link
Member Author

kitsonk commented Apr 6, 2017

I will take a look at what is in stores (and sorry again for not looking first) and see what it would take to do that.

@dylans dylans modified the milestones: 2017.04, 2017.05 Apr 29, 2017
@eheasley eheasley modified the milestones: 2017.05, 2017.06 Jun 6, 2017
@dylans dylans added this to the 2017.07 milestone Jun 6, 2017
@dylans dylans modified the milestones: 2017.07, 2017.08 Jul 29, 2017
Copy link
Contributor

@rorticus rorticus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few typos

src/compare.ts Outdated
}

/**
* An internal function that take a value from array being patched and the target value from the same
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

take takes

src/compare.ts Outdated
}

/**
* Compares to plain objects or arrays and return a set of records which describe the differences between the two
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to two

@kitsonk kitsonk merged commit 8f2209b into master Aug 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants