make clause-order extensible

[MichaelBlume]

This attempts what I suggested in a comment in #34

[jkk]

Thanks! I like the idea of making the core of the lib very extensible, so barring bugs or fundamental shortcomings, lib consumers can build what they need on top. Popular extensions could maybe be bundled into honeysql namespaces, leaving the core clean and simple.

[MichaelBlume]

Cool =)

So does this basically make sense? Does this look like an API we want people to use?

Also, do the priorities I've given (especially for the clauses I've marked as ties) seem correct?

[ddellacosta]

I like this in principal but I'm not super excited about the atom being used for configuration--I would much rather see this as an argument to functions somehow. As it is an atom implies a threading model, which is something I don't like the libraries I use to make assumptions about, as a general rule.

EDIT: following up on that--what about simply allowing an optional argument to format which would replace the default config?

EDIT 2: Sorry, re-reading the code I think I was confused--an arg to format wouldn't work. Will think on a good solution...

[MichaelBlume]

It occurs to me that we could use a var and alter-var-root instead -- which honestly is all defmethod/extend-protocol are under the hood. Would that be better?

[MichaelBlume]

Either way register-clause! is fundamentally stateful, and I get why that's bothersome, but defmethod and extend-protocol are basically stateful in the same way and we already encourage people to use those to extend honeysql

[ddellacosta]

EDIT: TL;DR: let's merge it, but I would rather remove the ^:private on clause-store ideally because I believe making things private generally causes more problems than it solves, and Clojure is about exposing your data.

Yeah, I mean I think some of it is also about making this explicit--I'm not generally a fan of private vars or mutations happening under the covers. I get your point about defmethod and extend-protocol, but the distinction there is that they are quite explicit in terms of how they work and are fundamental Clojure constructs.

That said I don't see a way around this at this time without expecting users to do some rather arcane hoop-jumpery with extend-type and ToSql so that they can pass in an alternate config, which is arguably worse. So I guess I would say, in the interests of moving forward, we should go with your patch until we figure out an obviously better way to do it, or if it ends up being an actual issue for any reason. I don't want to slow things down for reasons of ideological purity.

The only question I have remaining is if we want to ensure the configuration is thread-local or not. I guess the situation may arise that you want one configuration in one thread vs. another, but I suspect the probability of this happening is probably vanishingly small, and it is more likely the case that you'd be frustrated if you execute some HoneySQL code in a new thread and the previous thread's config doesn't get picked up...so probably what you have now makes the most sense.

I think the only thing I would like to push for is the removal of the ^:private annotation on the clause-store--if a developer is going to start messing with that themselves then they should be aware of the consequences, but making a var private (in my experience) just makes things harder to use and is of debatable benefit.

[jkk]

I share Dave's concerns, though I didn't peruse your proposed implementation thoroughly. Here are the options I can think of:

• Atom, registration
• Var alteration (not a fan)
• Multimethod
• Passing explicit configuration as arguments (probably not feasible)
• Dynamic var binding

For the multimethod option, maybe something like this? Are there drawbacks?

(defmulti get-clause-priority identity)
(defmethod get-clause-priority :default [_] Long/MAX_VALUE)
(defmethod get-clause-priority :select [_] 1)
(defmethod get-clause-priority :from [_] 2)
(defmethod get-clause-priority :where [_] 3)
;; etc

EDIT: As Michael notes, defmethod is inherently stateful, but at least it's consistent with how other extension mechanisms in the library work.

I think every clause should have a specific priority.

What if someone wants to insert a clause between two others? Use a float? Should we make that use case clear?

[MichaelBlume]

I went ahead and made all the priorities for the built-in clauses multiples of 10 for easier insertion (same reason BASIC programs always have line numbers in increments of 10...)

[MichaelBlume]

I'm a little concerned that an implementation using defmethod might wind up being slow, I'm going to do a bit of benching to check

[jkk]

Good point.

This is kind of a "power user" feature. As long as extension is possible, I don't feel strongly about the specific mechanism chosen

[MichaelBlume]

So defmethod seems to have a 10% performance hit compared with a var/atom registry. I wouldn't call that a deal-breaker and I'm up for using defmethod if you ( @jkk and @ddellacosta ) would like it better, but I think I'd prefer the registry.

Would it be any better if we renamed register-clause! to something like defclause or defpriority as a strong signal that it's a declarative statement intended for use at top-level in a namespace?

[MichaelBlume]

Just more or less rewrote this, the registry is back in an atom, it's public, and a lot of the logic has been simplified. There's another commit at MichaelBlume@defmethod which uses a defmethod instead of a registry.

[ddellacosta]

I'm on board--I propose we merge it so we can move forward, see if we run into issues, and review other possible solutions in the meantime. As it is I think this solves the problem perfectly adequately for now.