Better diff output when comparing

[slobo]

This shows a better diff of which keys are extra/missing rather than getting big strings dumped on the user

[slobo]

Can work on fixing up tests if you agree with the idea in principle. Thanks

[astorije]

@slobo, thanks for this!

Could you post outputs of a before/after your change, please? What does that change in practice, for those using the library?

Also, there are 2 other uses of .toString() in the file, do you think it's worth updating them as well?

[slobo]

Testcode:

 const A = Immutable.fromJS({ my: { very: { deep: 1, struct: 2, containing: 3, stuff: 4 } } })
 const B = A.setIn(['my', 'very', 'andmore'], 5)

 A.should.equal(B)

Without patch:

AssertionError: expected 'Map { "my": Map { "very": Map { "deep": 1, "struct": 2, "containing": 3, "stuff": 4 } } }' to equal 'Map { "my": Map { "very": Map { "deep": 1, "struct": 2, "containing": 3, "stuff": 4, "andmore": 5 } } }'
+ expected - actual

-Map { "my": Map { "very": Map { "deep": 1, "struct": 2, "containing": 3, "stuff": 4 } } }
+Map { "my": Map { "very": Map { "deep": 1, "struct": 2, "containing": 3, "stuff": 4, "andmore": 5 } } }

With patch:

AssertionError: expected { Object (my) } to equal { Object (my) }
+ expected - actual

 {
   "my": {
     "very": {
+      "andmore": 5
       "containing": 3
       "deep": 1
       "struct": 2
       "stuff": 4

[fresk-nc]

Nice PR! I have same problem

[astorije]

Hi @slobo, sorry I didn't get back to you earlier. Could you indeed look at test failures here?

And did you see my previous question:

Also, there are 2 other uses of .toString() in the file, do you think it's worth updating them as well?

[astorije]

@slobo, ping? :)
Are you still willing to take a look at this?

[slobo]

Yes, sorry it fell off my radar, let me take a look right away

[slobo]

I've updated the tests, let's see if they pass.

I didn't know how to deep check the messages (i.e. the + expected - actual .... bit ) - we only check the first line of exception - I documented expected output in the test, it may be a good idea to improve that bit.

Also, there are 2 other uses of .toString() in the file, do you think it's worth updating them as well?

The other two usages of .toString (that are in checks for .include and .keys) are I think appropriate. Currently messages looks like this:

AssertionError: expected 'List [ 1, 2, 3 ]' to include 4
AssertionError: expected 'Map { "foo": "bar", "hello": "universe" }' to contain key 'not-foo'

Using .toJS gives us less informative messages (it omits the type of immutable object):

AssertionError: expected [ 1, 2, 3 ] to include 4
AssertionError: expected { foo: 'bar', hello: 'universe' } to contain key 'not-foo'

So I suggest leaving those as-is.

[slobo]

@astorije, let me know if you need anything else before this can be merged. Thanks.

[slobo]

@astorije ping... thanks

[novwhisky]

Very interested in seeing this merged. Let me know if I can help.

[astorije]

Hey @slobo! Alright, I quickly looked at your code and comments from 1y+ ago and it looks good.
I can't believe I let that get stale for a year, sorry!!

I'll merge as-is and will have until v2 to find out if something is wrong.

Thanks for your help and patience!! 🙏