Correct various :arglists

[vemv]

Extracted from #331

Often, these functions assume a "context" coming from a -> call.

This context should be reflected in the :arglists - otherwise they stop being truthful and various tooling will display incorrect information.

It would also allow Eastwood to successfully lint tests.

[seancorfield]

This still has a _cxt arg in one change and it looks like many of the others are now treating the query arg as always present, instead of functions being multi-arity/variadic.

Getting the :arglists accurate for all the helpers is going to be tricky, given that the first argument is always an optional query -- so everything is variadic to some degree.

[vemv]

This still has a _cxt arg in one change

Whoops, amended. The typo (cxt <-> ctx) hindered the grepping 😄

it looks like many of the others are now treating the query arg as always present

I actually tried to apply some fine-grained reasoning here. The reasoning being: something like create-table never has a preceding query, since CREATE TABLE is a way to begin a SQL statement - not a fragment that may be present later.

Most fragments aren't like that: they are fragments that need a preceding fragment. e.g. ON CONFLICT alone isn't much useful, and while it technically may work without a query, in practice one never wants that. So it seems worth excluding it from the :arglists.

Disclaimer, I might be wrong here and there as I cannot claim comprehensive expertise of SQL syntax. My changes were driven from whenever Eastwood pointed out that the arglist was technically wrong.

Assuming the CI improvements from #331 , it seems a plausible path to begin by having technically correct arglists, and later keep progressing with more fine-grained arglists driven by insights that may grow organically.

[seancorfield]

Remember that HoneySQL helpers are (mostly) "order insensitive":

dev=> (-> (h/with-columns [:id :int :null])
 #_=>     (h/create-table :fruit)
 #_=>     (cond-> :some-cond (h/with-columns [:description :text]))
 #_=>     (sql/format))
["CREATE TABLE fruit (id INT NULL, description TEXT)"]

This is important when building SQL programmatically: that's why all of the helpers have an optional query (or at least sql) first argument.

I am concerned about a goal of "satisfy Eastwood" here as opposed to "accurately document argument lists" for (IDE) tooling -- I don't consider these to be entirely well-aligned -- which is why, right now, the helper :arglists document things fairly loosely and deliberately exclude the optional threaded DSL expression: because that's what is more useful for humans working with editors, for the most part. I'm going to leave this PR open for now and give it some more thought, but I'd rather approach this from a usability perspective than purely a linting perspective (I am more inclined to try to satisfy clj-kondo than Eastwood since the former is used for "live" linting while editing).

I've run into this sort of problem before with Eastwood -- I think it was with clojure.java.jdbc -- and I thought that the up shot was that Eastwood added some way to provide :arglists-equivalent metadata that was Eastwood-specific so library developers could satisfy Eastwood while also providing useful argument list descriptions for library users? Certainly, quite a few libraries use "pseudo-arglists" syntax that Eastwood does not understand -- compare the :arglists of several clojure.core functions with their actual signatures, for example.

[vemv]

Remember that HoneySQL helpers are (mostly) "order insensitive":

TIL!

This is important when building SQL programmatically: that's why all of the helpers have an optional query (or at least sql) first argument.

👍

I am concerned about a goal of "satisfy Eastwood" here as opposed to "accurately document argument lists" for (IDE) tooling

To be clear I don't consider satisfying a linter a goal in itself. Instead I consider it a tool for achieving a technically correct arglist that can help me when using Honey (I do from time to time depending on the job at hand). Often these actually end up improving things domain-wise; a great practical example was mcohen01/amazonica#445 which made arglists correct and far richer than before.

In our case, if an arg, which in fact can be used, is omitted from the :arglists, then I can get confused about the meaning of each argument in said arglist (because I won't know if the declared arg refers to the query, or to the specific thing).

So, it seems pretty achievable to add the optional query argument to every defn here as an additional arglist...?

- ([x])
+ ([x] [query x])

That would seem technically correct and would keep things usable (since one, as a user, can choose to omit the new arglist). Perhaps by keeping the new arglist in last position and/or naming the query arg _query instead, we would keep the average intended usage pretty clear.

Finally, a ns docstring could describe whatever convention is decided.

Armed with these we could avoid pseudo :arglists which I'm not atm convinced are an usual practice (clojure.core can rename arglists but keeps them technically correct AFAICT. e.g.)

[seancorfield]

Is arg* meaning zero-or-more instances of arg something that Eastwood understands? (from [xform* coll] -- so (eduction coll), (eduction xf coll), (eduction xf1 xf2 coll), etc are all valid)

And then we have stuff like this (for defn):

([name doc-string? attr-map? [params*] prepost-map? body] 
 [name doc-string? attr-map? ([params*] prepost-map? body) + attr-map?])

Does Eastwood understand that arg? means zero-or-one instance? What about the ... + form meaning one-or-more?

As I said, I'll keep this open while I think about it some more but I'm much less concerned about Eastwood liking the :arglists metadata than I am about what clj-kondo/LSP and inline code insight in editors do.

[seancorfield]

OK, I went digging into my notes about Eastwood and :arglists after confirming that clj-kondo and LSP seem to rely directly on the actual, declared argument lists...

You linked to eduction above, and I noticed jonase/eastwood#280 wherein Eastwood has to have a specific configuration so that it does not misreport calls to that function.

Here's one of the issues raised early on about Eastwood using :arglists for checking calls instead of the actual, declared arguments: jonase/eastwood#119 and here's the relevant part of the commit associated with that: jonase/eastwood@03d5086#diff-84edd4881ac6dcd54b5854371c7f3387996664724f605ecdbfec7be27b636b90R169 -- introducing a lot of configuration to deal with Hiccup and clojure.java.jdbc (the latter is where my memory of Eastwood's "weird" behavior comes from).

The current state of Eastwood's built-in configuration to address this is here: https://github.com/jonase/eastwood/tree/master/resource/eastwood/config (in three files).

This just feels wrong-headed to me so I'm not going to pander to Eastwood on this -- clj-kondo/LSP do "the right thing" and allow for arbitrary :arglists that help users, via documentation, which feel like the right approach to me. Sorry, but I'm closing this out.

[vemv]

You linked to eduction above [...]

Whoops, I unawarely chose a particularly tricky case (I didn't notice it used *). I've created jonase/eastwood#399 accordingly.

Anyway, :arglists using "regex" notation are only a fraction of honeysql's. So I don't think that consideration alone is enough to forget about the issue altogether.

Again, I'm not pushing for a fix just for satisfying a linter. Other tooling (like cider-nrepl) also makes use of :arglists. Omitting query doesn't seem exactly truthful or self-explanatory. I do get a possible rationale, but getting it / remembering it will be completely up to each programmer (we could consider a principle of least surprise here).

Hope these considerations help!

[seancorfield]

It's a useful discussion to have, so I appreciate it. It's inherently "hard" for deliberately variadic functions that can accept pretty much "anything" -- but I expect I'll continue to tune :arglists for honey.sql.helpers as I continue to work on the library.

[seancorfield]

Re: cider-nrepl -- does it use :arglists for code insight/inline help, or does it actually try to check function calls with the metadata? The former is fine -- it's what most editors do -- but the latter is problematic in my opinion (and I've always felt Eastwood made a mistake going down that path).

[vemv]

I'll continue to tune :arglists for honey.sql.helpers as I continue to work on the library.

That's great!

cider-nrepl -- does it use :arglists for code insight/inline help

I meant this - it simply uses arglists for displaying them along the docstring etc

(and I've always felt Eastwood made a mistake going down that path)

I've witnessed many factual errors in various codebases related to :arglists, detected by Eastwood. Namely, :arglists can fall out of sync relative to their originally intended usage.

So a tool that give true positives is useful. The occasional presence of false positives dosen't necessarily void that usefulness (and I plan to fix those FPs as described in jonase/eastwood#399).