Fork me on GitHub
#matcher-combinators
<
2024-01-11
>
Gilvan Reis17:01:32

Hello folks, I would like to create deep-equals function (`(m/match-with [map? m/equals] expected)`) on matcher-combinators because my team is replicating the same code some times and many colleagues (myself in the past included) though that m/equals was going to validate the internal map. So I would like to your opinions: 1. Does anyone have anything against doing this change? 2. Do you guys think this function fit matcher-combinators or do you think my team should implement that function internally? (I was looking to implement there mainly to make the usage easier) 3. Do you have any other name suggestion or this one is ok?

Phillip Mates18:01:06

Hey, thanks for bringing this up. It seems to be the thing that most trips folks up about the library. I think we need to find a way to communicate better the idea that datastructures have default matchers. I view it as the essence of the lib and it seems to be a common source of confusion. We could do this by improving the documentation, which I have the feeling is very verbose and hard to skim. It is also possible that adding another matcher like you are suggesting could help folks understand the lib better because they might stumble upon it and read the docstring / wonder why it is needed and piece together the context from there.

Phillip Mates18:01:11

You're suggesting something like the following in the matcher-combinators.matchers namespace:

(defn deep-equals [expected]
  (match-with [map? m/equals]
              expected))
and then using it like
(is (match? (m/deep-equals {:a :b})
            {:a :b :c :d}))

Phillip Mates18:01:00

I'm open to it if we can come up with a good name/docstring, though I'd love to hear what others think about it / if they have similar experiences and use-cases

Gilvan Reis18:01:34

> You’re suggesting something like the following Yeah, exactly like that and I can make the PR to add it and related docstring if you guys agree with this change

Gilvan Reis18:01:18

Internally we already had a discussion about why would we need this if we could use

(is (= {:a {:a1 :a2}}
       {:a :b :c :d}))
But using the deep-equals solution, we can have a way better error message, informing exactly what is the wrong key or value

Phillip Mates18:01:59

if you just need better error messages then maybe you can use https://github.com/lambdaisland/deep-diff2 which I think does a good job with that

Phillip Mates18:01:57

but if you want to combine matchers in a nested fashion, that is where matcher-combinators offers something deep-diff2 doesn't have

Gilvan Reis18:01:26

It is not just about the error message, but this is an advantage when compared with the = (just sharing the discussion)

👍 1
scottbale19:01:01

I'll share my opinion. Ultimately I'm okay with this addition. My experience, like many, was to be puzzled and irritated by m/equals behavior with maps, until I studied the documentation and understood the design and intent of default matchers for data structures and how that works with nesting. So I'm a little conflicted. On one hand, I wouldn't want the addition of something like deep-equals to be a substitute for properly understanding how matcher-combinators works, its design and intent. But like you said @U02G8N0EX44, the new function could serve as a learning aid especially if care is taken with the doc string to be consistent with the rest of the documentation. Possible docstring:

Return a matcher for the expected value which matches maps with `equals`. This solves a common need of matching nested maps with strict equality. See also: `matcher-for`.
Example usage:
(is (match? (deep-equals {:a :b}) {:a :b :c :d}))

👍 1
Gilvan Reis14:01:38

Hey folks, as we didn’t have any opinion against this change, I created the https://github.com/nubank/matcher-combinators/issues/217 and the draft https://github.com/nubank/matcher-combinators/pull/218 about this change. Feel free to comment on it, specially about the docstring content

👍 1
Gilvan Reis16:02:56

@U07PUGBA6 Which name do you prefer: strictly-equals or nested-equals I don’t have any strong opinion about them, so I would like to know a third opinion

scottbale19:02:35

@U06DKTFD692 I found @U02G8N0EX44 explanation of nested-equals in the PR to be very persuasive: it emphasizes that the key difference in behavior occurs in nested layers of a data structure (not the top layer).

❤️ 1
👍 1
Gilvan Reis20:02:51

@U02G8N0EX44 I got an 403 error when publishing to Clojars during the release: https://github.com/nubank/matcher-combinators/actions/runs/7747184131/job/21127130871 Is this a known error?

Phillip Mates20:02:27

never seen it before; maybe it is a new requirement from clojars (for a license file or something); I'm on vacation until March so don't really have time to dig in at the moment

👍 1
Gilvan Reis20:02:46

I will check it, thanks and good vacation

Gilvan Reis12:02:26

Hey folks, after some tries I finally reached the solution for the case, but I had to bump the tools.build for the :build (https://github.com/nubank/matcher-combinators/pull/220) Do you guys see any concern about bumping this dependency?

scottbale15:02:53

I can't think of any concerns I have. Thanks for digging in to this issue @U06DKTFD692!

Gilvan Reis17:02:04

It worked! the version is released on clojars https://clojars.org/nubank/matcher-combinators

woohoo 1
🎉 1