Skip to content
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

varying key order in maps should produce consistent diffs #48

Conversation

RutledgePaulV
Copy link
Contributor

fixes #47 and adds a test

@alysbrooks
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

Thanks @RutledgePaulV!

@alysbrooks alysbrooks merged commit 4d92566 into lambdaisland:main Jul 24, 2023
@plexus
Copy link
Member

plexus commented Feb 17, 2024

Released in v2.11.216

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

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.

Hashmap key order shouldn't matter
3 participants