search

lambdaisland / deep-diff2

Deep diff Clojure data structures and pretty print the result
Eclipse Public License 1.0
296 stars 18 forks source link

varying key order in maps should produce consistent diffs #48

Closed RutledgePaulV closed 1 year ago

RutledgePaulV commented 1 year ago

fixes #47 and adds a test

alysbrooks commented 1 year ago

Thanks, @RutledgePaulV! I'm wondering if you've done any performance comparisons before and after. It seems like this could be even faster, since it seems like there are fewer operations overall. (Certainly there's no code.) Obviously, correctness is more important than speed, but ideally this fix wouldn't cause a performance regression.

alysbrooks commented 1 year ago

Hi @RutledgePaulV will you have time to look at this in the near future? I think it's really close to being merged. If not, I can probably take this over and bring it over the finish line. Thanks!

RutledgePaulV commented 1 year ago

The brief repl tests I performed indicated there was no significant performance difference (either positive or negative). As you said, certainly there's very little going on in this revised implementation.

However, those tests are not scientific and ideally deep-diff2 would include a set of performance tests against various data sets. Those tests could help guard against performance regressions and provide optimization targets that can be improved on over time. I don't think this PR is the right place to introduce those :).

I added the comment you requested about why the [false false] case will never occur.

alysbrooks commented 1 year ago

Thanks @RutledgePaulV!

plexus commented 8 months ago

Released in v2.11.216

[lambdaisland/deep-diff2 "2.11.216"]                 ;; deps.edn
{lambdaisland/deep-diff2 {:mvn/version "2.11.216"}}  ;; project.clj