circleci / bond

spying for tests
127 stars 28 forks source link

Don't blow up when arglists metadata is missing #41

Closed j0ni closed 5 years ago

j0ni commented 5 years ago

Hey y'all :wave:

I recently encountered a problem with bond whilst trying to mock functions generated by HugSQL. I opened an issue on that tool too, because I think those functions should be correctly annotated with :arglists.

However, I also think that throwing is not the correct behaviour in the absence of an :arglists annotation. Would it be possible to have a generated warning, or something more benign than a fail? Alternatively, maybe a different error message which highlights the missing :arglists rather than claiming that any number of arguments is the wrong number of arguments?

I'll happily do up a PR if I get some guidance on your preferred solution.

marcomorain commented 5 years ago

👋 hi @j0ni

conormcd commented 5 years ago

This only happens with with-stub! and not with with-stub, right?

In this case, my personal preference would be to change the behaviour of with-stub! from

Like with-stub, but throws an exception upon arity mismatch.

to

Like with-stub, but throws an exception upon arity mismatch where it's possible to detect such a mismatch. Some generated functions don't include :arglists in their metadata. For those functions, this macro will behave exactly like with-stub.

or something similar.

I wouldn't necessarily object to a warning, but this library does not currently depend on any logging interface or implementation and it would be nice to keep the dependencies as light as they currently are.

If others feel queasy about silently passing then we could insert something like:

(when-not (:arglists (meta v))
  (throw (new #?(:clj IllegalArgumentException :cljs js/Error)
         "with-stub! may only be used on functions which include :arglists in their metadata. Use with-stub instead.")))

in the appropriate position.

j0ni commented 5 years ago

Hey @conormcd, either of those solutions sounds ok to me, tho I would prefer the second.

I'm a fan of crashing over programming errors as early as possible, and this seems more like a programming error than a runtime snafu.