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.

:thumbsup: Thanks for confirming. Regarding that behaviour; I think it makes sense; but I think in some cases there might be more nuance to it. For example, I think I’d like to tell duct/integrant (presumably through a new option/function to preserve backwards compatibility) to still allow any refsets to be empty; but to also guarantee that any keys which are referenced via an #ig/refset are instantiated automatically. i.e. to include (depth-search refset? sys) as keys to ig/init. The reason I’d like this is that right now I have to maintain a list of all derived-keys, and remember to supply them in every context we start the app from (each deftest, main, dev/repl, etc). It’s not a huge problem, but every time I’ve added a refset I’ve forgotten to include the key in the keys provided to main; which is more selective than our dev/repl environment because it doesn’t want to start everything). And I’ve only discovered these problems when I’ve built and deployed the app to a staging env. At the minute I think I’d always want this behaviour, not the current one. I can and will do the above to prevent this from happening again however I wonder if it’s a more sensible default for others too?!

@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?

Currently Duct relies on refsets working the way they currently do. In fact, that’s part of the reason they were added. Refsets allow for ordering without automatically pulling in keys. I’d need to consider how best to solve this without breaking existing behaviour, but this may take a while, as my focus is currently on Ring.

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.

Yeah no rush on my part, I can work around these issues… I’m really just raising them as hypothetical ideas. I’m curious about what you think would specifically break though? Imagine the new behaviour was opt in; would existing modules in duct core be broken if you instantiated them with this option?

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

Modules rely on refsets being able to be empty in order to allow ordering. They might work if a refset automatically included all its keys… however, I’m reticent to do that as it means removing existing functionality. I can’t remember if any existing modules or components rely on it.

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.

ok that makes sense… In principle I guess modules could still use the existing way; whilst the components in the prepped config could opt into the initialising of refsets. I can see why you might not want to do that though.

I see the current mechanism as being useful for modules and for prep-key in particular. They allow you to say “if there is a key K, include it here”.

So you can have optional behaviour such as, say, a logger or some kind of debug tracer.

But it’s also not too late to change this behaviour either. For example, by having something like #ig/refset {:key :user/foo, :opt? true}

(ig/refset :user/foo {:opt? true})

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.

Is it really :opt? true? here btw? As that is already the default (existing behaviour). Shouldn’t the option be more like :auto-init? true?

Yes, probably. I’m actually now thinking it’s might be better not to add init options to refs.

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:

Well it’s just an arbitrary flag right? It won’t actually do the initialisation; it’ll just flag that it should be included by ig/select-keys and/or some other arbitrary function/pred ?