Add support for Spec Alternatives

[rschmukler]

trafficstars

This commit introduces pluggable support for spec alternatives by introducing a configurable multi-method for ig/assert-pre-init-spec.

The :default implementation is to still use spec - but an advanced user can configure this to do whatever.

Although a user can already specify a custom pre-init validator using build many tools (eg. integrant.repl) call ig/init directly. This allows a user to get the same functionality without requiring them to rewrite everything from scratch.

One consideration is whether we should actually dispatch on anything for the multi-method or only ever use :default. I can't personally imagine a system needing to dispatch off of system, key or value, but let me know if you'd like it changed.

[rschmukler]

Looks like the build failed because of some dependency issues - tests passed locally for me.

[weavejester]

Thanks for the PR. This wasn't exactly the design I was thinking of, but I'll need to give this more thought before I commit to anything. This may take at a few weeks.

[weavejester]

The latest master branch should now be fixed for Travis.

[weavejester]

The design I'm considering is something like:

(defmulti assert-key
  (fn [key value] key))

(defmethod assert-key :default [key value]
  (when-let [spec (pre-init-spec key)]
    (when-not (s/valid? spec value)
      (throw (spec-exception key value spec (s/explain-data spec value))))))

(defn init [config keys]
   (build config keys init-key asset-key resolve-key)))

[rschmukler]

I believe that build expects the assert-key function to be in the shape of (f system key val) which is why I made the multi-method adhere to the same shape.

Happy to adjust the PR however you see fit!

[weavejester]

Yep, build would have to be adjusted as well. My example was just a rough sketch, rather than a complete solution. I'm still undecided on various details, like whether it should throw an exception, or just return a map describing an error.

[rschmukler]

I'd consider making it be responsible for throwing. This allows people to potentially disable throwing all together, as well as pre-process whatever they do end up throwing / printing (eg. via expound)

[abcdw]

Hi @weavejester, any ideas on how to proceed with this? Would like to use malli with integrant as well.

[weavejester]

@abcdw My last set of comments outlined a design, but I haven't implemented it, or received any code based on the design to review. I might be able to find time to take a look at it this weekend.

[abcdw]

@weavejester It would be very cool if you find some time. Also, I've made a patch according to your suggestions.

From 378cd87bc3951685044990e999bc49e39d3c177b Mon Sep 17 00:00:00 2001
From: Andrew Tropin <[email protected]>
Date: Fri, 9 Dec 2022 12:28:44 +0400
Subject: [PATCH] Add assert-key multimethod

---
 src/integrant/core.cljc | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/src/integrant/core.cljc b/src/integrant/core.cljc
index aa44e67..8bb995d 100644
--- a/src/integrant/core.cljc
+++ b/src/integrant/core.cljc
@@ -307,7 +307,7 @@
 
 (defn- build-key [f assertf resolvef system [k v]]
   (let [v' (expand-key system resolvef v)]
-    (assertf system k v')
+    (assertf k v')
     (-> system
         (assoc k (try-build-action system f k v'))
         (vary-meta assoc-in [::build k] v'))))
@@ -319,7 +319,7 @@
   An optional fourth argument, assertf, may be supplied to provide an assertion
   check on the system, key and expanded value."
   ([config keys f]
-   (build config keys f (fn [_ _ _])))
+   (build config keys f (fn [_ _])))
   ([config keys f assertf]
    (build config keys f assertf (fn [_ v] v)))
   ([config keys f assertf resolvef]
@@ -348,7 +348,7 @@
 (defn expand
   "Replace all refs with the values they correspond to."
   [config]
-  (build config (keys config) (fn [_ v] v) (fn [_ _ _]) resolve-key))
+  (build config (keys config) (fn [_ v] v) (fn [_ _]) resolve-key))
 
 (defmulti prep-key
   "Prepare the configuration associated with a key for initiation. This is
@@ -404,20 +404,22 @@
 
 (defmethod pre-init-spec :default [_] nil)
 
-(defn- spec-exception [system k v spec ed]
+(defn- spec-exception [k v spec ed]
   (ex-info (str "Spec failed on key " k " when building system\n"
                 (with-out-str (s/explain-out ed)))
            {:reason   ::build-failed-spec
-            :system   system
             :key      k
             :value    v
             :spec     spec
             :explain  ed}))
 
-(defn- assert-pre-init-spec [system key value]
+(defmulti assert-key
+  (fn [key value] key))
+
+(defmethod assert-key :default [key value]
   (when-let [spec (pre-init-spec key)]
     (when-not (s/valid? spec value)
-      (throw (spec-exception system key value spec (s/explain-data spec value))))))
+      (throw (spec-exception key value spec (s/explain-data spec value))))))
 
 (defn prep
   "Prepare a config map for initiation. The prep-key method is applied to each
@@ -438,7 +440,7 @@
    (init config (keys config)))
   ([config keys]
    {:pre [(map? config)]}
-   (build config keys init-key assert-pre-init-spec resolve-key)))
+   (build config keys init-key assert-key resolve-key)))
 
 (defn halt!
   "Halt a system map by applying halt-key! in reverse dependency order."
@@ -472,7 +474,7 @@
             (if (contains? system k)
               (resume-key k v (-> system meta ::build (get k)) (system k))
               (init-key k v)))
-          assert-pre-init-spec
+          assert-key
           resolve-key)))
 
 (defn suspend!
-- 
2.38.1

[weavejester]

Thanks for the patch, though now that I've had a chance to look at it, I realize that changing assertf in any way is a bad idea. My current thought is to wrap assert-key in something that catches and wraps the exceptions. I'll need to write some code to see if that makes any sense.