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

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

Closed
j0ni opened this issue Oct 16, 2018 · 3 comments
Closed

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

j0ni opened this issue Oct 16, 2018 · 3 comments

Comments

@j0ni
Copy link

j0ni commented Oct 16, 2018

Hey y'all 👋

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

👋 hi @j0ni

@conormcd
Copy link
Member

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

j0ni commented Oct 16, 2018

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.

conormcd added a commit that referenced this issue Oct 26, 2018
If a function was generated without `:arglists` metadata then it can't
be stubbed using `with-stub!` since `stub!` can't verify the arity of
the replacement function.

Instead of giving an inscrutable error message, we now assert that
`:arglists` exists and produce an error message which points the user in
the right direction.

Fixes: #41
conormcd added a commit that referenced this issue Oct 26, 2018
If a function was generated without `:arglists` metadata then it can't
be stubbed using `with-stub!` since `stub!` can't verify the arity of
the replacement function.

Instead of giving an inscrutable error message, we now assert that
`:arglists` exists and produce an error message which points the user in
the right direction.

Fixes: #41
conormcd added a commit that referenced this issue Oct 26, 2018
If a function was generated without `:arglists` metadata then it can't
be stubbed using `with-stub!` since `stub!` can't verify the arity of
the replacement function.

Instead of giving an inscrutable error message, we now assert that
`:arglists` exists and produce an error message which points the user in
the right direction.

Fixes: #41
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

No branches or pull requests

3 participants