weavejester / integrant

Micro-framework for data-driven architecture
MIT License
1.24k stars 64 forks source link

Throw an error on not found methods #6

Closed metametadata closed 7 years ago

metametadata commented 7 years ago

I was surprised to not get an error on undefined init-key/halt-key! methods for the particular key. Looking at code, this seems to be an intended behaviour: to silently treat keys without methods as plain maps which don't need initialisation/halting logic. What could be the use-case for such feature? I expect most, if not all, keys in the config/system correspond to stateful objects which implement start/stop behaviour.

  1. I'd like to suggest removing :default cases from multimethods to force the user to explicitly specify all the needed methods. This is a less surprising behaviour and helps to early on detect cases when namespace with defined methods for the key was mistakenly not required.

  2. In case the "plain map" pattern must also be supported library could implement the ig/consthelper, e.g.:

(def config
  {:adapter/jetty {:port 8080, :handler (ig/ref :handler/greet)}
   :handler/greet {:name "Alice"}}

   ; just a bag of stuff that will be added into system as is
   :bag-of-dependencies (ig/const {:foo 123 :server (ig/ref :adapter/jetty) :handler (ig/ref :handler/greet)}))
weavejester commented 7 years ago

I've been wondering about this.

The idea behind the defaults is that it allows a user to split up a configuration arbitrarily, without needing to write multimethods for references to immutable data. For example, maybe you want to reference the server port in multiple locations:

{:adapter/jetty {:port #ref :server/port}
 :client/config {:port #ref :server/port}
 :server/port   8080}

The disadvantage is of course that configurations can fail silently. I'm leaning toward the idea that good error reporting is more important than ease of splitting up a configuration.

metametadata commented 7 years ago

I see, this feature helps with writing EDN files with less code duplication because there're no let-statements/vars in EDN. I suppose it should be possible to write something like this to support such scenarios and have error reporting about missing methods:

{:adapter/jetty {:port #ref :server/port}
 :client/config {:port #ref :server/port}
 :server/port   #const 8080}
weavejester commented 7 years ago

Yes, though I'm not sure about the use of #const, because it's the key rather than the value that determines how the value is processed.

Let me give this a bit of thought.

dsteurer commented 7 years ago

I like the current behavior. I would think that systems often have components that don't require state management but still have dependencies and configuration options. In this case the default behavior of init-key and halt-key! works well. This behavior also matches the behavior of the Component library. Users who prefer a more conservative behavior of init-key and halt-key! could also overwrite the :default implementation to raise an exception.

weavejester commented 7 years ago

I'm leaning toward removing the default behavior for init-key, but keeping the default behavior for the other multimethods. As all keys must have an init-key implementation, this would raise an error if the multimethod was not loaded, while minimising the amount of code you'd have to write.

Obviously there's a tradeoff here. If we have a default init-key method we save a line of code for each key that we don't need to initiate. But on the other hand, missing out a require or getting the key name wrong can result in very unintuitive errors, especially since maps act as functions in Clojure.

vspinu commented 7 years ago

Would it be possible to implement #asis reader macro to mark identity keywords? Or maybe provide a macro to automatically generate identity init-key for a list of keywords?

weavejester commented 7 years ago

I don't think using a reader tag is the right approach. Tags are a way of making data more specific, whereas an #asis tag defines out the data is interpreted by code.

I'm not sure that a macro would add much that doseq doesn't already, so I'd rather not do that.

One option would be to use something about the keys to denote identity. For example, maybe there's a reserved namespace we can use:

{:integrant.static/port 3000
 :ring.adapter/jetty {:port #ig/ref :integrant.static/port}}

However, I'm not convinced that we should have many static keys in a configuration.