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

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

Open
NoahTheDuke opened this issue Apr 27, 2023 · 2 comments · May be fixed by #207
Open

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

NoahTheDuke opened this issue Apr 27, 2023 · 2 comments · May be fixed by #207

Comments

@NoahTheDuke
Copy link

NoahTheDuke commented Apr 27, 2023

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

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

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

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 a pull request may close this issue.

3 participants