Clojurians Log v2

Yes, that’s deliberate. A refset is valid even if it’s of zero size, and allows for optional dependencies. The documentation could be better though.

@weavejester: I wrote this as part of my reply on the other thread; but thought I’d post it separately to keep the discussions separate: Regarding optionality you mention ( https://clojurians.slack.com/archives/C52HVRVE1/p1583972397018400?thread_ts=1583945583.018300&cid=C52HVRVE1 ) ; I hadn’t really thought much about it until now; but now you mention it I can see that might be really useful. Do you think it might be worth considering having an #ig/optref key to make optionality more explicit, and decomplecting it from being a set of values? Where an ig/optref would represent a single component not a set; and not fail the system if it wasn’t defined?

I’m not crazy about having an #ig/optref because that would also necessitate a #ig/optrefset , and I’d rather have some other mechanism that doesn’t involve adding a symbol per combination. #ig/opt #ig/ref might be a possibility. Or ^:ig/opt #ig/ref. But as my focus is on Ring, it’s unlikely that I’ll be looking into this for a few months at least.

Yeah I definitely agree a generic opt reader would be a better idea

Also this isn’t something I have an immediate need for… though I’m sure if the feature was there I would find uses for it.

Is it worth me opening an issue for it on integrant?

I think there is already an issue for this.

👀

Ok this one seems closest: https://github.com/weavejester/integrant/issues/77

will need to read through the discussion later to see how close it comes

Yes, that’s the issue I meant. Though I guess we could also add another issue as well.

Yeah might be more explicit

I’ll need to consider the best approach for this. Making refsets automatically pull in keys removes existing functionality and breaks backward compatibility. So it’s something that I’m reticent to do even if Integrant hasn’t reached 1.0. At the very least I’d want to retain that functionality.

Yeah I’m not advocating breaking compatibility. I guess I’m first trying to establish if in principle and with hindsight we think this would have been a good idea. And if we think it is; then whether it might be worth adding in a non-breaking way. I think it would probably be a nice feature to have… I’m not yet sure whether it would be worth adding. Also it’s worth stating we could in principle cover it by just adding a note on how to wire up in the README… Or perhaps just exposing a new function like this would be enough?

(defn find-refset-keys
  [config]
  (->> config
       (depth-search ig/refset?)
       (map :key)))

My current thought is that the behaviour is logical, but not intuitive. A set can have zero elements, therefore you don’t need to pull in any keys in a refset. A single ref must have one element, therefore keys have to be pulled in for it to work.

However, there’s been two people who have been confused by this behaviour, and I can see why it might not be obvious.

Essentially people assume dependencies brought in by ig/init are maximal, when they’re actually the minimal set.

The reason this was chosen is that you can always add keys to ig/init, but there’s no mechanism to exclude them.

I understand what you’re saying; but I think what I’m saying would still allow the set to have 0 elements. It’s just about asking integrant to guarantee that if there are keys that belong to the refset; that they will be initialised.

Does integrant error if a key in the keys isn’t found in the system? I can’t recall…

👀

No, it doesn’t.

Perfect then, in that case what I’m saying would work, no?

You can always add keys to ig/init, but you can’t exclude keys, so if Integrant went the more intuitive route of including all keys of a refset, that would mean you couldn’t exclude keys in a refset that you don’t want.

So that removes functionality. We’d either need to add an option to ig/init to allow keys to be excluded, or exclude them before ig/init with a dissoc, or add some option to the refs to allow you to distinguish between them.

Potentially the issue might be that ig/init shouldn’t choose keys. Perhaps that needs to be a separate function that strips down the configuration to what’s nececssary.

Yeah, I was just going to say; isn’t this only an issue if you rely on the 1 arity ig/init ?

Something like: (ig/init (ig/select-keys config [:duct/daemon]))

And then there could be maximal or minimal strategies.

Something to think about at least… I do like the idea of decomplecting ig/init…

> Potentially the issue might be that ig/init shouldn’t choose keys. Perhaps that needs to be a separate function that strips down the configuration to what’s nececssary. Yeah, this is really what I am getting at. If the user selects the keys; then integrant doesn’t need anything funky like exclusions. Users should just state the keys they want to try to init. Integrant could provide a mini API for finding such keys; depth-search with a pred is essentially that already. A strategy as you describe it is essentially just finding the right set of keys.

Currently the user does select the keys explicitly; Integrant only includes keys that the user doesn’t specify if the init would fail without them.

But separating that out into its own function might be a ‘simpler’ solution. So init just cares about initiation, and some other function like ig/select-keys cares about the keys themselves.

Equally there could be ig/exclude-keys if you wanted to take the opposite approach.

Just to check; you originally spotted this issue because Duct uses (ig/init config [:duct/daemon]) , correct?

Just checking we’re talking about the same thing… So “`ig/init-2`” would take 2 args a config and keys. keys would already be normalised, and in the case of the user wanting a derived key; by the time the user calls ig/init-2 they would have needed to convert the ancestor key into the specific instance keys, mentioned in the system?

Ah, no, we’re not talking about the same thing. I’m talking about reducing init to 1 arity.

Ok same difference really

So instead of writing (ig/init config keys) you’d write (ig/init (ig/select-keys config keys))

Yeah that works too — might be marginally better than what I’m saying too; but it decomplects in the same way is my point.

Yeah.

You ran into this issue while using Duct, right?

Yes, sorry was going to return to that

Though it’s not just because of :duct/daemon we have another key we need to list too at the minute (this one is a bit of a hack just now; ultimately we should refactor some component deps to make it disappear and just happen as part of the dependency ordering — but effectively this key loads some initial data into the database) And then there a small handful of other keys we list because of the refset issue.

Okay. If you create an issue for this then it’ll remind me to come back to this after I’ve had a think about the best solution.

Yeah, I’ll try and create an issue for it this evening or tomorrow. Thanks also for entertaining this discussion btw, it’s been really useful and insightful :thumbsup: :thumbsup:. We make huge use of integrant and duct at Swirrl; and though at times I might sound like its fiercest critic; make no mistake we’re huge fans of it. Returning briefly to what we were discussing… to summarise are you saying that? - “`ig/init-2`” in this model, would always start every key in the config map it is given. - A prior step which you’ve referred to as ig/select-keys is responsible for returning the sys config where everything will be initialised. If this is the case then is ig/select-keys enough? Don’t we need a suite of functions here? ig/select-keys presumably wants to select keys on the basis of exact match or their hierarchy; but would we not want other functions to handle #ig/opt dependencies and ensure that they are appropriately included or stripped from the sys config? Sorry, not expecting to design this all here — but it feels like this has opened another design can of worms :thinking_face: