weavejester / integrant

Micro-framework for data-driven architecture
MIT License
1.22k stars 63 forks source link

Add support for custom assertf definitions #94

Open futuro opened 2 years ago

futuro commented 2 years ago

Hello!

Thank you for making Integrant, I've started adopting it and I love how simple it is to use and how clearly it defines dependencies between my components!

I'm also using Malli, and I was hoping to leverage Malli schemas with the pre-init-spec multimethod, but it looks like assert-pre-init-spec only works with Spec.

Are you open to a PR to update init and resume to take another argument, allowing callers to provide a different assertion function, with assert-pre-init-spec being the default?

Thanks! Evan

weavejester commented 2 years ago

Originally, this is what build was for, as init is just a wrapper around that. But resume is a little more complex.

I'll need to consider what the best way to solve this is. It's not a bad idea to make it more generic, but I don't think adding an argument is the best way to solve it.

One possible solution is to break backward compatibility while we're not yet 1.0, and make a more generic pre-init method, and move pre-init-spec into an integrant.spec namespace, perhaps.

danielytics commented 2 years ago

I was just thinking about this too and see that #60 made an attempt, but was never completed.

Since pre-init-spec is only called by assert-pre-init-spec, which is an internal function, that's the only thing that needs to be replaceable. With that said, I'm personally in favour of moving spec-specific code into its own namespace.

If assert-pre-init-spec is replaced with a generic assert-pre-init, which by default calls the existing assert-pre-init-spec function, then its up to the user to decide whether their override calls a pre-init-spec equivalent or something else.

For overriding assert-pre-init, and I know this may go against the design aesthetic you have in mind, I would personally be in favour of keeping it really simple and just making it a global that you can override by mutating in some way. The simplest options that I thought of (but you may think of other better ones) are:

  1. Providing a (ig/set-assert-pre-init! my-f) helper that replaces assert-pre-init using alter-var-root. Simple and effective, since it does exactly what it says on the tin: replaces the function.
  2. Storing it in something mutable like a volatile whose value you can replace to be the function you want to call instead.
  3. Making it a dynamic variable that you can override with binding. This could be abstracted away in integrant-repl so that the REPL experience remains unchanged.
  4. Of course, just providing additional overloads for init and resume with an extra argument as mentioned above, is also an option and avoids making it a global.

Since this only affects init and resume, which are typically only called in a single place (your application bootstrapping logic) or from the REPL (which can be streamlined by integrant-repl), I think a simple approach is probably the best. Not necessarily the above, but something that avoids adding any extra machinery. There's no need for more complex overriding logic (#60 mentioned per-key validation for example, this can still be provided by a custom assert-pre-init if its desired, but I don't think Integrant needs to necessarily do it out of the box).