Skip to content

Conversation

@kevindew
Copy link
Contributor

Hello 👋, thanks for your work on this gem. We've found it useful.

This PR introduces an array_path option that can be used when generating a diff. This represents the path to aspects of the diff as an array rather than a string.

eg.

x = {'a' => 1}
y = {'a' => 2}

HashDiff.diff(x, y)
=> [["~", "a", 1, 2]]h

HashDiff.diff(x, y, :array_path => true)
=> [["~", ["a"], 1, 2]]

This allows there to be more flexibility with the types used as keys in a hash. Allowing workarounds for issues such as: #25

eg

x = {'a'=>1}
y = {:a=>1}
HashDiff.diff(x, y)
=> [["-", "a", 1], ["+", "a", 1]]
HashDiff.diff(x, y, :array_path => true)
=> [["-", ["a"], 1], ["+", [:a], 1]]

And improved ability to patch hashes with keys:

eg

x = {a: {b: :c}}
y = {a: {b: :d}}
diff = HashDiff.diff(x, y)
=> [["~", "a.b", :c, :d]]
HashDiff.patch!(x, diff)
    NoMethodError: undefined method `[]=' for nil:NilClass
diff = HashDiff.diff(x, y, array_path: true)
=> [["~", [:a, :b], :c, :d]]
HashDiff.patch!(x, diff)
=> {:a=>{:b=>:d}}

This updates the patch! and unpatch! methods to accept diffs with either paths as strings or as arrays.

The patches suggested in the README only work when you have string keys
on the hash.
Repository owner deleted a comment from codecov-io Jul 29, 2017
@liufengyun
Copy link
Owner

Thanks a lot @kevindew, this feature is what I always want to have. This PR is also related to #29 .

One question I have in particular is: if the path is represented as arrays, how we know that a number in the array is a key or an index to an array? Otherwise, this PR looks great.

@kevindew
Copy link
Contributor Author

Oh wow, sorry I really don't know how I missed that other PR in my reading up 🤦‍♂️ Would you prefer it if I closed this one and we work at getting that one over the line?

One question I have in particular is: if the path is represented as arrays, how we know that a number in the array is a key or an index to an array?

We technically don't, but I don't think this actually causes us a problem. I actually started by making this clearer by storing array indexes in arrays e.g "a[0]" => ["a", [0]] but that had the downside that you had to say "supports all hash indexes except array ones" (as uncommon as they are).

If we say have two hashes that look similar but is comparing a hash and an array, the diff includes the type difference:
eg:

> HashDiff.diff({ a: [1] }, { a: { 0 => 1 } }, array_path: true)
=> [["~", [:a], [1], {0=>1}]]

It could potentially catch you out on a custom_compare function where you couldn't distinguish between a key of "*" and an array where the key isn't worked out:

> diff = HashDiff.diff({ "*" => [{ a: "b" }, { b: "c" }] }, { "*" => [{ a: "b" }, { c: "d" }]}, array_path: true) { |path, a, b| puts path.inspect; nil }
[]
["*"]
["*", "*"]
["*", "*", :a]
["*", "*"]
["*", "*", :b]
["*", "*", :a]
["*", "*"]
["*", "*", :a]
["*", "*", :c]
["*", "*"]
["*", "*", :b]
["*", "*", :c]
["*", 0]
["*", 0, :a]
=> [["-", ["*", 1], {:b=>"c"}], ["+", ["*", 1], {:c=>"d"}]]

I'm assuming that's not a big deal as people probably have a reasonable idea of what their structure is like that they're diffing.

For patch! when we have an integer key we check whether it's an array. You could have problems with a patch if you change the type of object that is used for input:

> diff = HashDiff.diff({ a: [0, 1, 2] }, { a: [0, 1, 3] }, array_path: true)
=> [["-", [:a, 2], 2], ["+", [:a, 2], 3]]
> HashDiff.patch!({ a: { 1 => 4, 2 => 5 } }, diff)
=> {:a=>{1=>4, 2=>3}}

But there's all sorts of ways you can cause problems by changing the input object for patch!

@liufengyun
Copy link
Owner

@kevindew Thanks a lot for the clarification. I think as long as the users are warned, it's fine to keep it as it is. Could you please add a special comment to mention the tricky issue? Otherwise, LGTM.

@codecov-io
Copy link

codecov-io commented Aug 3, 2017

Codecov Report

Merging #34 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #34      +/-   ##
==========================================
+ Coverage   99.21%   99.27%   +0.05%     
==========================================
  Files          10       10              
  Lines         637      688      +51     
==========================================
+ Hits          632      683      +51     
  Misses          5        5
Impacted Files Coverage Δ
lib/hashdiff/lcs.rb 100% <100%> (ø) ⬆️
lib/hashdiff/patch.rb 100% <100%> (ø) ⬆️
spec/hashdiff/patch_spec.rb 100% <100%> (ø) ⬆️
lib/hashdiff/diff.rb 95.69% <100%> (+0.19%) ⬆️
lib/hashdiff/util.rb 98.5% <100%> (+0.17%) ⬆️
spec/hashdiff/diff_spec.rb 100% <100%> (ø) ⬆️

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 7271736...04e4e8b. Read the comment docs.

This introduces an array_path option that can be used when generating a
diff. This represents the path to aspects of the diff as an array rather
than a string.

eg.
```
x = {'a' => 1}
y = {'a' => 2}

HashDiff.diff(x, y)
=> [["~", "a", 1, 2]]h

HashDiff.diff(x, y, :array_path => true)
=> [["~", ["a"], 1, 2]]
```

This allows there to be more flexibility with the types used as keys in
a hash. Allowing workarounds for issues such as:
liufengyun#25

eg
```
x = {'a'=>1}
y = {:a=>1}
HashDiff.diff(x, y)
=> [["-", "a", 1], ["+", "a", 1]]
HashDiff.diff(x, y, :array_path => true)
=> [["-", ["a"], 1], ["+", [:a], 1]]
```

And improved ability to patch hashes with keys:

eg
```
x = {a: {b: :c}}
y = {a: {b: :d}}
diff = HashDiff.diff(x, y)
=> [["~", "a.b", :c, :d]]
HashDiff.patch!(x, diff)
    NoMethodError: undefined method `[]=' for nil:NilClass
diff = HashDiff.diff(x, y, array_path: true)
=> [["~", [:a, :b], :c, :d]]
HashDiff.patch!(x, diff)
=> {:a=>{:b=>:d}}
```

This updates the `patch!` and `unpatch!` methods to accept diffs with
either paths as strings or as arrays.
s/comparisions/comparisons
@kevindew
Copy link
Contributor Author

kevindew commented Aug 3, 2017

@liufengyun Sure thing - I've added additional documentation to it. Oh and also caught a typo in the readme.

@liufengyun
Copy link
Owner

LGTM, thanks a lot @kevindew 👍

@liufengyun liufengyun merged commit 5c47aff into liufengyun:master Aug 6, 2017
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.

3 participants