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

pre-init-spec for composite keys #33

Closed
julienfantin opened this issue Dec 8, 2017 · 13 comments
Closed

pre-init-spec for composite keys #33

julienfantin opened this issue Dec 8, 2017 · 13 comments

Comments

@julienfantin
Copy link

julienfantin commented Dec 8, 2017

My expectation is that a value under a composite key should somehow conform to the specs returned by pre-init-spec for its component(s).

Using your example:

{[:group/a :adapter/jetty] {:port 8080, :handler #ig/ref [:group/a :handler/greet]}
 [:group/a :handler/greet] {:name "Alice"}
 [:group/b :adapter/jetty] {:port 8081, :handler #ig/ref [:group/b :handler/greet]}
 [:group/b :handler/greet] {:name "Bob"}}

I'd want to be able to define a pre-init-spec method for :handler/greet and :adapter/jetty:

(defmethod ig/pre-init-spec :adapter/jetty [_]
  (s/keys :req-un [::port ::handler]))

(defmethod ig/pre-init-spec :handler/greet [_]
  (s/keys :req-un [::name]))

But right now this will only work if I define four other specs, one per composite all returning spec(s) defined for the components.

(defmethod ig/pre-init-spec :adapter/jetty [_]
  (s/keys :req-un [::port ::handler]))

(defmethod ig/pre-init-spec :handler/greet [_]
  (s/keys :req-un [::name]))

(defmethod ig/pre-init-spec [:group/a :adapter/jetty] [_]
  (ig/pre-init-spec :adapter/jetty))

(defmethod ig/pre-init-spec [:group/b :adapter/jetty] [_]
  (ig/pre-init-spec :adapter/jetty))

(defmethod ig/pre-init-spec [:group/a :handler/greet] [_]
  (ig/pre-init-spec :handler/greet))

(defmethod ig/pre-init-spec [:group/b :handler/greet] [_]
  (ig/pre-init-spec :handler/greet))

That's a lot of code for what seems like an 80% use-case?

What this allows however is to fully customize the spec for any given composite which might be desirable sometimes I guess.

Maybe a middleground would be to use the spec returned by the composite key if there is one, and if there is none fallback to building an s/and of (map ig/pre-init-spec composite-key) or alternatively use the first spec found, but that leaves the question of which one should be used?

@weavejester
Copy link
Owner

Composite keys are just keys that inherit from all of their keywords. If we wanted some way of combining specs, then we can't use multimethods, as inheritance doesn't combine.

I've actually written a library for combined inheritance, but whether this is the best way of handling things for Integrant I'm not certain, as it introduces a new dependency and new syntax.

@weavejester
Copy link
Owner

Your example doesn't appear to match up to the rest of the issue. If you have a configuration:

{[:group/a :adapter/jetty] {:port 8080, :handler #ig/ref [:group/a :handler/greet]}
 [:group/a :handler/greet] {:name "Alice"}
 [:group/b :adapter/jetty] {:port 8081, :handler #ig/ref [:group/b :handler/greet]}
 [:group/b :handler/greet] {:name "Bob"}}

And specs:

(defmethod ig/pre-init-spec :adapter/jetty [_]
  (s/keys :req-un [::port ::handler]))

(defmethod ig/pre-init-spec :handler/greet [_]
  (s/keys :req-un [::name]))

Then it should work fine; I just tried it. It's only when you have multiple specs for the same keyword that the inheritance is forced to pick one.

@julienfantin
Copy link
Author

@weavejester I'm not sure I understand because the two defmethods you quoted above never get invoked for that config. pre-init-spec dispatches on the four composite keys.

@julienfantin
Copy link
Author

Composite keys are just keys that inherit from all of their keywords. If we wanted some way of combining specs, then we can't use multimethods, as inheritance doesn't combine.

I think I understand that constraint better now. Even my cursory glance at the problem already hinted at some sort of hierarchy-driven spec composition mechanism, which seems like it will inevitably lead to some tradeoffs.

But while this behavior is self-explanatory when looking at the source code, I think it should be mentioned in the README and/or the pre-init-spec docstring.

On the other hand I'm now wondering what kind of hooks could be provided so that if I have N composite keys, that should all somehow use the same spec from one of their components, I could define a single method for that hook rather the N boilerplate methods.

@weavejester
Copy link
Owner

@weavejester I'm not sure I understand because the two defmethods you quoted above never get invoked for that config. pre-init-spec dispatches on the four composite keys.

Ah, you're right. There's a bug; pre-init-spec doesn't normalize the composite keys first.

@julienfantin
Copy link
Author

julienfantin commented Dec 8, 2017

@weavejester I actually noticed that discrepancy but ig/normalize-key returns something like :integrant.composite/group.a+handler.greet_35275 which seemed like something that would be used for an internal dispatch mechanism.

Could you describe what you think the behavior should be from the perspective of a user trying to write pre-init-spec methods on a composite key?

@weavejester
Copy link
Owner

The behaviour should be the same as all the other multimethods, in that composite keys should be treated as keywords that derive from all the keywords contained in the vector.

In other words, this:

[:group/a :handler/greet]

Should have the same semantic meaning as:

(derive :temp/a :group/a)
(derive :temp/a :handler/greet)
:temp/a

@julienfantin
Copy link
Author

julienfantin commented Dec 8, 2017

I'm sorry it's still not clear to me what method(s) would get dispatched for that composite key, and if there are more than one, how would the different specs get combined?

@weavejester
Copy link
Owner

It's the same rules as normal multimethods. The most specific key is chosen, and ties are broken by prefer-method.

So in the above case we have a key that's derived from :group/a and :handler/greet. Clojure will look for a multimethod that handles either of those. If there is a method for both, it uses prefer-method to determine which to use. If neither has a multimethod, it looks at their parents, walking down the ancestors until it finds one with a method.

weavejester added a commit that referenced this issue Dec 8, 2017
@weavejester
Copy link
Owner

Commit e97c4b3 should fix the dispatch of pre-init-spec.

@julienfantin
Copy link
Author

Thanks! Any chance you could cut a release for this fix?

@julienfantin
Copy link
Author

Awesome, thank you @weavejester

@weavejester
Copy link
Owner

Released 0.6.2

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