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

Add support for custom assertf definitions #94

Open
futuro opened this issue Jan 20, 2022 · 2 comments
Open

Add support for custom assertf definitions #94

futuro opened this issue Jan 20, 2022 · 2 comments

Comments

@futuro
Copy link

futuro commented Jan 20, 2022

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

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

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).

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