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

Support per-arity :args and :fn specs #14

Open
alexandergunnarson opened this issue Mar 21, 2018 · 8 comments
Open

Support per-arity :args and :fn specs #14

alexandergunnarson opened this issue Mar 21, 2018 · 8 comments

Comments

@alexandergunnarson
Copy link

alexandergunnarson commented Mar 21, 2018

First of all, Orchestra is great! It's extremely useful, and employs some ideas that are similar to what I'm currently pursuing in one of my libraries.

One feature set I would love to see is to be able to add :args and :fn validation on a per-arity basis:

(defn-spec sym whole-fn-ret-spec
  ([a arg-spec-a, b arg-spec-b]
    {:pre  (s/and a b) ; to be `s/and`ed to the other arity-specific `:args` specs
     :post ((if a number? string?) %)} ; arity-specific `:fn` spec
    ...))

What do you think about this?

@jeaye
Copy link
Owner

jeaye commented Mar 21, 2018

@alexandergunnarson Thanks for the ticket and I'm glad you're liking Orchestra. I haven't tried it yet, but try giving Clojure's normal :pre and :post a shot, in combination with defn-spec. You're able to specify the meta map after the return spec.

This is how :fn is specified with defn-spec as well. For example:

(defn-spec func' number?
{:fn #(= (:ret %) (-> % :args :meow))}
[meow number?]
(Math/abs meow))

EDIT: On second thought, you mean per-arity :pre and :post, so that's likely not going to help you. Will need to give that more thought. If your actual use case is like your example, then it may be outside the scope of Orchestra, which is focused solely on adding and facilitating complete instrumentation with spec. Your example, to me, is less about arity-specific additional predicates and more about argument-specific function specs in general.

EDIT 2: I gave :pre and :post a shot with defn-spec, as described above. It does behave as intended. Again, you may've not been asking that, but I wanted to be complete.

user=> (require '[orchestra.core :refer [defn-spec]])
nil
user=> (defn-spec my-abs pos-int? [n integer?] {:pre [(zero? n)]} (if (< n 0) (- n) n))
user/my-abs
user=> (my-abs 2)

java.lang.AssertionError: Assert failed: (zero? n)

@alexandergunnarson
Copy link
Author

alexandergunnarson commented Mar 21, 2018

Thanks for your quick reply @jeaye! Yes, my actual use case is structurally similar to my example, the most important part being (s/and a b) (:pre), and secondarily the ((if a number? string?) %) (:post). I appreciate what you mentioned in Edit 2, but I was hoping for not just an AssertionError, but actual specs that give more helpful errors as well as generative tests. Perhaps it would be clearer if I showed what I envision the final :args spec being in the case I posted initially:

;; Has an automatically-generated function spec of this:
{:args (s/or :arity-1 (s/and (s/cat :a arg-spec-a :b arg-spec-b)
                             #(s/and (:a %) (:b %))) ; this is the part that I'm proposing
 :ret ...}

:fn is insufficient for this use case because that spec is called after the function has returned, not before the function is called, which is not ideal.

That takes care of the arity-specific :pre spec. The arity-specific :post spec is conceptually similar; it could conceivably be automatically tacked on to the :fn spec via an s/and.

Maybe this is outside the scope of Orchestra as you suggest, but for me it's a pretty essential addition if I'm to use defn-spec and have specs that relate multiple arguments (rather than just individual argument specs like a string?, b number?). I could of course write this sort of spec with fdef but I would lose the terseness of defn-spec.

@alexandergunnarson
Copy link
Author

Still interested in this feature — what are your thoughts @jeaye?

@jeaye
Copy link
Owner

jeaye commented Apr 11, 2018

Thanks for the ping. I see the use in this, but it's not something I can make time to implement right now, since it's not something I would use. If you or someone else were to implement it, I think it would have to be different from your original suggested syntax, since what you suggested is ambiguous for empty fn definitions (does it return a map or is that map specifying :pre and :post metadata?).

The syntax which would work, I think, would be to use the existing meta map already supported in defn and defn-spec and to use an Orchestra-specific key for providing "metadata" for each arity. Something like this:

(defn-spec sym whole-fn-ret-spec
  {:arity {2 {:pre (s/and a b) ; to be `s/and`ed to the other arity-specific `:args` specs
              :post ((if a number? string?) %)}}} ; arity-specific `:fn` spec
  ([a arg-spec-a, b arg-spec-b]
    ...))

That way Clojure can still have its own :pre and :post in defn-spec, which doesn't know of spec, while each arity can have spec-integrated :pre and :post which get combined with s/and, as you suggested.

Let me know if that suggestion suits all of your needs.

@alexandergunnarson
Copy link
Author

alexandergunnarson commented Apr 11, 2018

No problem! And I see what you're saying — I've never liked the :pre and :post syntax for that reason. I prefer the following syntax:

(defn-spec sym whole-fn-ret-spec
  ([a arg-spec-a, b arg-spec-b & args | (s/and a b args) > #((if a number? string?) %)]
    ...))

The | is meant to evoke notions of "such that" from set builder notation in mathematics. The > is meant to evoke notions of a return value, as in an a->b conversion fn in Clojure. And :pre and :post are left untouched. Of course, this means | and > can't be used as symbols for fn bindings, but neither can & for that matter — they would behave similarly in that sense.

In either case I understand you don't have time to implement it. Once we hammer out the syntax, I could conceivably make the time for it.

@alexandergunnarson
Copy link
Author

This is done in https://github.com/alexandergunnarson/defnt.

@jeaye
Copy link
Owner

jeaye commented May 11, 2018

I've just read through the docs you have for defnt, @alexandergunnarson. Nice work!

As for its intersection with Orchestra, if you're up for making a PR, please do! Ideally we don't have too many of these defn + spec macros floating around, each only slightly different.

@alexandergunnarson
Copy link
Author

alexandergunnarson commented May 11, 2018

Thanks so much for taking the time to do that @jeaye! I appreciate your positive feedback :) And ideally, yes haha, we'd just have one. I'm not sure about PR'ing it, though, as then a significant chunk of code will have to be maintained in two places. As I see it there are four options, in decreasing order of my preference:

  1. Excise defn-spec from Orchestra (or deprecate it) and refer users to use the defns macro within the defnt repo. This shouldn't mean your work on it gets erased from collective memory; I will definitely continue to mention it in defnt as a related work, as should you in orchestra. In any case I would love to collaborate on defns/defnt.
  2. Alias defns as defn-spec within Orchestra and excise the original defn-spec code. This would introduce a dependency on defns.
  3. Keep the original defn-spec code and optionally reference defns as an alternative. This would proliferate spec'ed defn flavors which is not ideal.
  4. Excise the original defn-spec code (or deprecate it) and replace with PR'd defns code. This would make it so we have to maintain two large chunks of identical code across two repositories which is, in my opinion, the furthest from ideal out of any of these situations.

What do you think?

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

2 participants