liufengyun / hashdiff

Hashdiff is a ruby library to to compute the smallest difference between two hashes
MIT License
560 stars 63 forks source link

ignore_keys ignores only common keys #88

Closed rellampec closed 3 months ago

rellampec commented 9 months ago
require 'hashdiff'
Hashdiff::VERSION
#=> "1.1.0"

The following snippet fails to ignore top level key :a

a = { a: 4, b: {a: 5, c: 6} }
b = {       b: {a: 7, c: 3} }

diff = Hashdiff.diff(a, b, ignore_keys: :a)
#=> [["-", "a", 4], ["~", "b.c", 6, 3]]

Shouldn't diff be expected to be the below?

[["~", "b.c", 6, 3]]

The documentation on ignore_keys states that:

_The :ignore_keys option allows you to specify one or more keys to ignore, which defaults to [] (none). Ignored keys are ignored at all levels._

Here line where :ignore_keys happens (in Hashdiff::ComparableHashes::call):

opts[:ignore_keys].each { |k| common_keys.delete k }

added_keys and deleted_keys don't seem to have the exclusion applied.

rellampec commented 8 months ago
a = { a: 4, b: {a: 5, c: 6}         }
b = {       b: {a: 7, c: 3},  d: 8 }

(1) ignore_keys: keep backwards compatibility

diff = Hashdiff.diff(a, b, ignore_keys: [:a, :d])
#=> [["-", "a", 4], ["~", "b.a", 5, 7], ["~", "b.c", 6, 3], ["+", "d", 8]]

(2) new ignore_added_keys: only affects added keys

diff = Hashdiff.diff(a, b, ignore_keys: [:a, :d])
#=> [["-", "a", 4], ["~", "b.a", 5, 7], ["~", "b.c", 6, 3]]
MatzFan commented 3 months ago

@rellampec thanks for spotting this. It is an oversight on my part and having just discovered the bug in my own project today I'm happy to provide a fix, sorry for the delay.

@liufengyun I'd be inclined to provide a new method (not_keys)? with the originally intended functionality and issue a deprecation warning for ignore_keys and remove references to it from the docs. This should not break anything using ignore_keys which can hang around for as long as you deem fit. Let me know your thoughts before I proceed.

liufengyun commented 3 months ago

Thank you for looking into it @MatzFan @rellampec and sorry for the late response.

Given that the spec says that ignored keys should ignore at all levels, we can consider the current behaviour is a bug. I'd prefer a simple bug fix unless there is a strong reason to do otherwise.

MatzFan commented 3 months ago

@liufengyun would you consider a patch release to Ruby Gems with this fix? Thx.

liufengyun commented 3 months ago

I forgot to mention, it's already released in v1.1.1: https://www.rubydoc.info/gems/hashdiff#ignore_keys