Skip to content

Apply clj-kondo metadata to let-dataset macro #162

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

radovanne
Copy link

@radovanne radovanne commented Aug 18, 2024

Issue

While using tablecloth, clj-kondo doesn't apply proper linting to let-dataset macro.

Reason for fix

Clj-kondo by default doesn't recognise custom macros which makes it unpleasant to work with because it trows Unresolved symbol error where it shouldn't be.

Solution

As far as I know there are three possible ways to make clj-kondo behave properly.

  1. Add {:clj-kondo/lint-as 'clojure.core/let} metadata to macro.
  • this metadata tells clj-kondo to lint let-dataset as if it were a clojure.core/let.
  1. Add {:clj-kondo/ignore [:unresolved-symbol]} metadata to macro.
  • any error symbol can be placed in the list and clj-kondo will ignore it, :unresolved-symbol is just in our case.
  1. Adjusting local .clj-kondo config, but that would resolve issue only for me.

Why I chose this approach

  • I chose first approach and not the second one is because If we use second approach with ignore metadata, language-server-protocol fails to work inside that let-dataset binding. I was not able to:
  1. See binding signature.
  2. Jump to definition.
  3. Jump to or list references where that binding is used.
  • With first approach let-dataset macro behaves like normal let binding, that being said it will also show if some binding is not being used!
    eg:
(tc/let-dataset [x (range 1 6)
                 y 1
                 z (dfn/+ x y)])

In this example clj-kondo will inform us that binding z is not being used.

  • If by any chance we don't want clj-kondo to inform us about unused binding we can add additional metadata key to metadata map.
(defmacro let-dataset
  {:clj-kondo/lint-as 'clojure.core/let
   :clj-kondo/ignore [:unused-binding]}  <--
  ([bindings] `(let-dataset ~bindings nil))
  ([bindings options]
   (let [cols (take-nth 2 bindings)
         col-defs (mapv vector (map keyword cols) cols)]
     `(let [~@bindings]
        (dataset ~col-defs ~options)))))

Question:

  • Because of my inexperience with tablecloth and how it is used I am not sure if people would prefer to see which binding is not being used ( normal let binding behaviour ) or would they prefer to not see which binding is not being used?
  • Adding underscore to binding name made clj-kondo ignore unused-binding like it should but I don't think that people would do that because that binding name will be visualised by Clay as column name or value.
  • If people would prefer to not see that unused binding info from linter then I can add additional metadata.

Reason:
While using tablecloth clj-kondo doesn't apply proper linting to
let-dataset macro.
Clj-kondo by default doesn't recognise custom macros which makes it
unpleasent to work with because it trows `Unresolved symbol` error
where it shouldn't be.

Solution:
As far as I know there are three possible ways to make clj-kondo behave properly.
1. Add `{:clj-kondo/lint-as 'clojure.core/let}` metadata to macro.
** this metadata tells clj-kondo to lint let-dataset as if it were
a clojure.core/let.
2. Add `{:clj-kondo/ignore [:unresolved-symbol]}` metadata to macro.
** any error symbol can be placed in the list and clj-kondo will ignore
it, :unresolved-symbol is just in our case.
3. Adjusting local .clj-kondo config, but that would resolve issue only for me.
Add metadata to correct file
@radovanne radovanne closed this Aug 18, 2024
@radovanne
Copy link
Author

radovanne commented Aug 18, 2024

exporter/write-api! doesn't build macro as intended, it doesn't put macro metadata on it.
I will reopen when it is ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant