nubank / matcher-combinators

Library for creating matcher combinator to compare nested data structures
Other
467 stars 23 forks source link

Bad usage stubs for `match?` and `thrown-match?` #206

Open NoahTheDuke opened 1 year ago

NoahTheDuke commented 1 year ago

clojure.test/assert-expr is clever but slightly magical, which makes it harder to work with: Where does match? come from? How does it work? Why aren't there any docstrings? Why does Clojure throw an "unable to resolve symbol: match?" error if I use this outside of is?

My proposed solution (as seen in clojure-expectations.clojure-test) is to create dummy functions with descriptive docstrings and meaningful argument names that can be referred but will throw exceptions if used outside of is calls:

(defmacro bad-usage [msg]
  `(throw IllegalArgumentException ~msg))

(defn match?
  "The `matcher` should be the matcher-combinator represented the expected value,
   and the `actual` should be the expression being checked."
  [matcher actual]
  (bad-usage "match? must be used inside of `is`.))

This way, a user can write:

(ns example.foo
  (:require
    [clojure.test :refer [deftest is]]
    [matcher-combinators.test :refer [match?]]
    [matcher-combinators.matchers :as m]))

(deftest test-matching-with-explicit-matchers
  (match? (m/equals 37) (+ 29 8))
  (is (match? (m/regex #"fox") "The quick brown fox jumps over the lazy dog")))

And they'll get a reasonable error message: "match? must be used inside of `is`." instead of the less helpful message Syntax error compiling at (example/foo.clj). Unable to resolve symbol: match? in this context.

If this is reasonable, I'd be willing to open a PR with these changes.

acamargo commented 1 year ago

I like your proposal! I agree that clojure.test/assert-expr is magical indeed, and I can see that error message being helpful for those who do not understand how things work under the hood.

philomates commented 1 year ago

I also like it; definitely gives some context to the magic that happens via clojure.test/assert-expr. :+1: for a PR with these changes