limit and offset helper funcitons have incorrect arglists

[mrkam2]

limit and offset arglists are missing the first map argument being currently defined as:

{:arglists '([limit])}

This causes eastwood linter errors in our project for otherwise valid code:

wrong-arity: Function on var #'honey.sql.helpers/limit called with 2 args, but it is only known to take one of the following args: [limit]

[seancorfield]

I've always said Eastwood is wrong to use :arglists metadata for linting instead of the actual argument lists. This is a long-standing problem with Eastwood. clj-kondo does this properly.

:arglists is intended to provide guidance for humans -- all of the helpers are variadic with [& args] and all of the :arglists explicitly provided are intended to show the non-DSL arguments. That is not going to change.

[mrkam2]

explicitly provided are intended to show the non-DSL arguments

What do you mean by "non-DSL arguments"? It's not clear from the documentation that it accepts both 1 or 2 arguments and ignores everything beyond 2nd argument, or even 1st, if it is not a map.

(honey.sql.helpers/offset 10)
{:offset 10}
(honey.sql.helpers/offset 10 20)
{:offset 10}
(honey.sql.helpers/offset {} 10 20)
{:offset 10}

I find this as confusing for humans as it confuses eastwood.

[mrkam2]

@seancorfield bump

[seancorfield]

I honestly don't know what to say in response to this given the examples in the docs that show the helpers having the DSL data structure threaded through them.

[mrkam2]

If eastwood were using real argument list, it would have never noticed any issues if this function was incorrectly used with 3 or more arguments, for example. It does a good job of using :arglists as the source of more specific information about the real argument options.

Moreover, clojure docstrings become insufficient as user have to go to examples and other documentation to figure out how this has to be used. This may be referred to as unnecessary mental load.

Having something like {:arglists '([limit], [query limit])} doesn't look as too confusing for people and would solve both the documentation and linting.

[mrkam2]

@seancorfield bump

[seancorfield]

I've already said "I honestly don't know what to say in response to this". Stop being rude and entitled.

[mrkam2]

I've already said "I honestly don't know what to say in response to this". Stop being rude and entitled.

I'm sorry, I didn't mean to be rude or entitled. I pinged you in case you've missed my last comment.

[seancorfield]

I want to be crystal clear that this issue is closed and I will have no further discussion on this.

Eastwood has had issues with arglists dating back around seven years at this point and has generally dealt with it by accruing custom configuration to deal with third-party libraries that provide :arglists metadata for human readable hints, that don't match actual defn argument lists. You can see that it has already added configuration for many honey.sql.helpers functions starting here: https://github.com/jonase/eastwood/blob/master/resource/eastwood/config/third-party-libs.clj#L349

You can see an earlier discussion about Eastwood and HoneySQL here: #334

And this Eastwood issue intended to address some of Eastwood's limitations in this area: jonase/eastwood#399

Since Eastwood has configuration for HoneySQL, you should go raise an issue on Eastwood's repo to have that existing configuration updated to include all the helpers that have been added since that configuration was last updated.

As noted (repeatedly, in many places, over many years), I think Eastwood is wrong to use :arglists for this sort of thing. LSP and clj-kondo parse defn argument lists and therefore lint code correctly.

No need to respond here.