kwrooijen commented 4 years ago

Problem

I'd like to create my own multimethods, which most likely will not fit in integrant itself. But in order to do that I need access to the normalize-key function.

Solution

Make normalize-key public. This way I can do something like the following

(defmulti my-init-key
  (fn [key _] (ig/normalize-key key)))

(defn my-init
  ([system]
   (my-init system (keys system)))
  ([system keys]
   (ig/build system keys my-init-key)))

This makes it easier for people to extend integrant, without having to create a pull request.

weavejester commented 4 years ago

I'll need to think about this one. The normalize-key method is internal to Integrant, and making it public ensures that I can't change how composite keywords are processed. I'd need some strong use-cases in order to justify such a change.

kwrooijen commented 4 years ago

I see, I didn't think about it that ways, sorry about that.

Wouldn't you be able to create a major release (breaking change) if the behavior changes? Of course that would be extra overhead for the project.

Maybe there's another way to solve this? My goal is to make integrant more extendable, since it seems that I need a lot of extra features that don't really fit in core.

weavejester commented 4 years ago

After listening to some of Rich's talks on the matter, I'm become convinced that breaking changes shouldn't happen under almost any circumstance.

However, I've noticed that composite-keyword is already public, when I had thought it was private. Given this, I don't think there's any reason normalize-key shouldn't be public as well, as it's just a thin wrapper over composite-keyword.

A public function needs a docstring, but apart from that I think I'm fine with it.

kwrooijen commented 4 years ago

After listening to some of Rich's talks on the matter, I'm become convinced that breaking changes shouldn't happen under almost any circumstance.

I don't recall having seen any of those, I probably should though.

@weavejester While writing a docstring for this function I realized that maybe it would be good to use composite-key? instead of vector? in this function? Also some other additions..

(defn normalize-key
  "Return a composite keyword if `k` is a composite key. If `k` is a qualified
  keyword, return `k`. If `k` is neither a composite-key nor a qualified
  keyword, an invalid key exception is raised."
  [k]
  (cond
    (composite-key? k) (composite-keyword k)
    (qualified-keyword? k) k
    :else (throw (invalid-key-exception k))))

I think it makes sense to add this extra :else case, since this can be used publicly. It would help the user find issues faster in the case if misuse, especially since this function doesn't accept all types of arguments.

I'd still need to write invalid-key-exception, but I wanted to see if you're ok with this change, or if you have a better idea.

weavejester commented 4 years ago

Let's use a precondition instead: {:pre [(valid-config-key? key)]}

kwrooijen commented 4 years ago

@weavejester I've applied your requested change. Since we're adding a precondition, any chance you've thought about how to solve this problem? https://github.com/weavejester/integrant/pull/70#issuecomment-571331463

weavejester commented 4 years ago

Probably just by throwing an exception. I've added a comment to the PR.

I think this commit looks okay, but let's change the docstring slightly:

  "Given a valid Integrant key, return a keyword that uniquely identifies it."

Everything else is an implementation detail.

weavejester / integrant

Make the normalize-key function public #76

Problem

Solution