Skip to content

add remove-unchanged API #42

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Mar 30, 2023
Merged

add remove-unchanged API #42

merged 7 commits into from
Mar 30, 2023

Conversation

humorless
Copy link
Member

Try to solve the issue #13 with an API remove-unchanged, test, CHANGLOG

@humorless humorless requested a review from plexus March 28, 2023 16:33
Copy link
Member

@alysbrooks alysbrooks left a comment

Choose a reason for hiding this comment

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

Looks good, but there's some cleanup needed, I think.

If you wanted to add some property-based/generative testing, you could add a test that verifies that, for any data structure, diffing it against itself and then applying remove-unchanged returns an empty structure.

@humorless
Copy link
Member Author

Thanks @alysbrooks so much.
After I tried to run the property test, I found out that there are a lot of edge cases which I did not think it throughly.

I can not make this property test pass until now.
However, I have considered more about the semantic of remove-unchanged and I changed it a lot.

(defspec diff-itself 100
  (prop/for-all [x diff-test/gen-any-except-NaN]
                (if (coll? x)
                  (= (manipulate/remove-unchanged (ddiff/diff x x))
                     (empty x))
                  (= (manipulate/remove-unchanged (ddiff/diff x x))
                     nil))))

@plexus @alysbrooks
Do you think that I should dig it throughly to figure out why this property test can not work? Or the current remove-unchanged you think is good enough?

@plexus
Copy link
Member

plexus commented Mar 30, 2023

What do the failing cases look like?

@humorless
Copy link
Member Author

What do the failing cases look like?

FAIL in cljs:lambdaisland.deep-diff2.manipulate-test/diff-itself (cljs/test.js:429)
Expected:
  {:result true}
Actual:
  {:shrunk
   {:total-nodes-visited 756,
    :depth 51,
    :pass? false,
    :result false,
    :result-data nil,
    :time-shrinking-ms 168,
    :smallest [#{0 1 false -6 -5 -4 -3 -2 -1}]},
   :failed-after-ms 70,
   :num-tests 59,
   :seed 1680177300377,
   :fail
   [#{1.1197369622161879e-14 1.3019822841584656e-21 0.6875
      #uuid "a907a7fe-d2eb-482d-b1cc-3acfc12daf55"
      -30
      :X/*!1:3
      :u7*A/p?2IG5d*!Nl
      :**d7ws
      "ý"
      "ÔB*àñS�¬ÚûV¡ç�¯±·á£H�
                            �û?'V$ëY;CL�k-oOV"
      !U-h_C*A7/x0_n1
      A-*wn./o_?4w18-!
      "ìêܼà4�^¤mÐðkt�ê1_ò�· À�4\n@J\"2�9)cd-\t®"
      y3W-2
      #uuid "6d507164-f8b9-401d-8c44-d6b0e310c248"
      "M"
      :cy7-3
      :w4/R.-s?9V5
      #uuid "1bcb00c9-88b9-4eae-9fea-60600dfaefa0"
      -20
      #uuid "269ab6f9-f19d-4c9d-a0cb-51150e52e9f7"
      -235024979
      :O:m_9.9+A/N+usPa6.HA*G
      228944.657438457
      :x/w?
      :__+o+sut9!t/?0l
      "�â��«"
      false
      #uuid "b6295f83-8176-47b5-946e-466f74226629"
      e3zQ!E*5
      :T5rb
      :++y:2
      -7364
      zG/ex23
      "¡"
      -4318364480
      :D+?2?!/Hrc!jA7z_2
      :z-I/!8Uq+d?
      -0.5588235294117647
      -0.5925925925925926
      -0.8108108108108109}],
   :result false,
   :result-data nil,
   :failing-size 58, 
   :pass? false, 
   :test-var "diff-itself"}

@plexus
Copy link
Member

plexus commented Mar 30, 2023

The problem is not in remove-unchanged, but in deep-diff itself. It seems for some sets we do some weird things:

(let [s #{false 5}]
  (ddiff/diff s s))
;; => #{{:- 5} false {:+ 5}}

@alysbrooks
Copy link
Member

A little surprised that bug hasn't been discovered before. Maybe it's just sets with a mixture of types that have issues?

@plexus
Copy link
Member

plexus commented Mar 30, 2023

Logged a separate issue, seems it also happens for maps. I think the false is part of the problem here.
#43

plexus added 2 commits March 30, 2023 11:12
Make sure we don't report a difference when there is none. For sets the issue
came from handling them as ordered collections when they aren't, with maps we
had a case of using sets as predicates, which breaks when one of the elements is
false or null.

Closes #43
@plexus
Copy link
Member

plexus commented Mar 30, 2023

I renamed the function and namespace to mimize, and added a top-level entrypoint.

@humorless seems you're all good now, the problem wasn't your code or the test, the problem was already there, but it's fixed now.

@plexus plexus merged commit 34d5e17 into main Mar 30, 2023
@plexus plexus deleted the laurence/add-remove-unchanged branch March 30, 2023 15:45
@plexus
Copy link
Member

plexus commented Mar 30, 2023

Two remarks,

  • you have a diff-item? where you check with instance? but then later on you were checking for :- or :+, isn't that a problem if the user passes in a map with :+ or :- key?
  • you were checking for diff-item? or map keys, but map keys can also be arbitrary values, so we need to also check that none of their children have diff items.

Actually, now that I write this I'm not sure if we descend into map keys when diffing...

Anyway, great work, happy to have this feature. Next we can think about how to expose this in kaocha.

@humorless
Copy link
Member Author

The two remarks are totally true. I only thought of the most typical cases when implemented.

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