Skip to content
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

Offer a Clojure API #226

Closed
vemv opened this issue Jun 3, 2019 · 18 comments
Closed

Offer a Clojure API #226

vemv opened this issue Jun 3, 2019 · 18 comments
Labels
enhancement New feature or request

Comments

@vemv
Copy link
Contributor

vemv commented Jun 3, 2019

Is your feature request related to a problem?

I like to run formatters/linters in the following way:

(clojure.tools.namespace.repl/refresh :after 'user/format!)

Where user/format! invokes cljfmt et al (via their Clojure APIs), and since very recently, clj-kondo too.

I used main for that, however that ns is not an API, as you reflect in the README, so I had to hack things a little.

Describe the solution you'd like

An api ns (or api.alpha, if you will) which would expose a function similar to main but with the following improvements:

  • Options are passed as a hashmap, not as a CLI-style arg vector
  • No (or optional) profiling
  • State is guaranteed to be fresh between runs (I see a TODO: move global state to local context)

Describe alternatives you've considered

There's the alternative of disregarding the Reloaded integration, but it's a formula that has worked well for me for quite long now, with ~10 formatters/linters integrated. Git integration and multithreading speed things up.

So I'm happy with the vanilla JVM and with avoiding Graal as long as possible, which would represent another yet moving piece in the stack.

Btw, as expected Kondo is really fast! Certainly fast enough to not get in my way in Reloaded.

WDYT?

Cheers - V

@borkdude
Copy link
Member

borkdude commented Jun 3, 2019

Hi @vemv,

You can already use clj-kondo from Clojure like this:

$ clj
Clojure 1.10.0
user=> (require '[clj-kondo.main :as kondo])
nil
user=> (kondo/main "--lint" "src")
linting took 620ms, errors: 0, warnings: 0

You'll have to pass in strings as arguments, but the command line API can be considered stable.
Can you explain what the hack was you had to do?
The profiling is off unless you set a certain environment variable, so don't worry about that.
The state is fresh, except for the configuration (this was only a problem in unit testing and already fixed on master).

So out of the box, clj-kondo on the JVM should already do what you want, except for having a Clojure-y API instead of passing strings. Do I understand this correctly?

Note that when running in a JVM with a newer tools.reader version than rewrite-clj uses:
clj-commons/rewrite-clj#76
This is one reason why I like to run the binary: no dependency conflicts.

@vemv
Copy link
Contributor Author

vemv commented Jun 3, 2019

Hi @borkdude , thanks for the reply!

You'll have to pass in strings as arguments, but the command line API can be considered stable.

Builing argument vectors and pr-string stuff felt fragile. Note that I disable a couple linters and also I run the linter over specific files, not src. So things got relatively unwieldy.

Can you explain what the hack was you had to do?

  • The mentioned arg buiding
  • Creating a copy of defn main without profiling (turned out to be unnecessary, thanks for the clarification)
  • Also removing linting took %sms, which becomes noisy in a Reloaded workflow

The state is fresh, except for the configuration (this was only a problem in unit testing and already fixed on master).

Saw that one, ace! (I had done the adaptation work a few days before that)

Note that when running in a JVM with a newer tools.reader version than rewrite-clj uses

Arrgh, unfortunate one. Too bad rewrite-clj is currently on a hiatus.

Personally I can live with it atm (nothing at work needs the latest tools.reader), and maybe by the time I need to upgrade, rewrite-clj will be fixed as well (or in any case: hacking seems possible)


As a last observation, the current API does not leverage clojure.spec which would seem adequate here, ensuring options are correctly passed.

@borkdude
Copy link
Member

borkdude commented Jun 3, 2019

Summary of Slack discussion with @vemv:

I will document the output format which can then be filtered (#230).
The only remaining issue is using strings to call the main function (as if you're calling it like a command line tool). You can consider the way you're calling the command line tool the official contract. Supporting an official Clojure API (a hashmap instead of a seq of strings) to be used on the JVM will maybe also raise questions like: don't output to stdout, but return hash-maps. So this needs more thought and feedback from people who want a similar feature I guess.

@vemv
Copy link
Contributor Author

vemv commented Jun 3, 2019

Just for the sake of persistence, other interesting points that popped up were:

  • introducing specs (to be consumed on demand), even if the binary won't leverage it
  • Introducing an in-memory cache
    • for reduced intrusiveness to the user
    • also for increased statelessness across runs
    • API consumers would be free to pass an already-filled cache, for squeezing some perf

As a last observation, I think the current main could be split into two: the inner part, concerned with EDN in, EDN out, and the outer part, which adds profiling, reporting, and all actual println ing.

Would address both my perceived needs, and those expressed in https://twitter.com/simon_brooke/status/1130382052996669441 , while keeping Kondo's codebase DRY (i.e. I'm not particularly asking for adding more code, but for splitting/refactoring certain code).

@simon-brooke
Copy link

Hi, I'm sort-of working (that is to say, writing experimental conceptual code) on a unified wrapper for existing Clojure code quality tools. It's really not ready for other folk to look at yet, but for the purposes of this discussion I've put it up here. The important document is probably this one.

Regardless of whether my tool ever becomes useful or not, it does seem to me that it would be valuable for tools which generate commentary on Clojure code to output a common, standardised, easily parseable notation, in order that that notation can be consumed by other tools.

@simon-brooke
Copy link

@borkdude, @vemv would you have a wee look at my proposed EDN and propose changes?

@borkdude, would you accept a patch to clj-kondo which generated this EDN as an option?

@vemv
Copy link
Contributor Author

vemv commented Jun 3, 2019

would you have a wee look at my proposed EDN and propose changes?

Reviewed! Idea and format seem sound to me.

I have a linter-wrapping tool with ~6 months of (occasional) progress. I don't think it competes with your idea/goals and one project could benefit from the other. Will ping you as I open-source it (this/next week)

@borkdude
Copy link
Member

borkdude commented Jun 3, 2019

@simon-brooke To answer your question about accepting a PR: to save you time, it's probably better if I add the EDN output, since I'm refactoring the main namespace quite a bit. Would you like to have this printed to stdout or returned as values?

@borkdude
Copy link
Member

borkdude commented Jun 3, 2019

@vemv I've got a first version here: #231
Welcome to test. Example:

$ clj
Clojure 1.10.0
user=> (require '[clj-kondo.main :as clj-kondo])
nil
user=> (def res (clj-kondo/run! {:files ["src"]}))
#'user/res
user=> (:findings res)
({:type :redundant-do, :message "redundant do", :level :warning, :row 360, :col 1, :filename "src/clj_kondo/main.clj"} {:type :redundant-do, :message "redundant do", :level :warning, :row 361, :col 3, :filename "src/clj_kondo/main.clj"})
user=> (clj-kondo/print-findings! res)
src/clj_kondo/main.clj:360:1: warning: redundant do
src/clj_kondo/main.clj:361:3: warning: redundant do
nil

@simon-brooke With this in place, it's trival to output EDN. I may rename the :type key to something else. Maybe :linter?

@simon-brooke
Copy link

That looks fine to me. In conjunction to the pull request I've just submitted, I think we have something. I'd leave :type as it is - after all, it is the type of the issue found, and I think that may be useful for other tools too. Although it may lead later to a need for a registry of issue-types, or at least some way of telling whether an issue from clj-kondo of type :redundant-do is of the same type as an issue from kibit with type :froboz.

@simon-brooke
Copy link

(:type would also be useful for doing i18n on issue messages.)

@borkdude borkdude added the enhancement New feature or request label Jun 4, 2019
@borkdude
Copy link
Member

borkdude commented Jun 4, 2019

Added {:output {:format :json}}, {:output {:format :edn}} and {:output {:summary false}} (for not printing the summary at the end) options. I'll move the "JVM API" functions to clj-kondo.core.

Screenshot 2019-06-04 09 40 53

@borkdude
Copy link
Member

borkdude commented Jun 4, 2019

@vemv The API now looks like this:

(require '[clj-kondo.core :as clj-kondo])
(def findings
  (:findings
   (clj-kondo/run!
    {;; seq of string or file
     :files ["corpus" (io/file "test")]
     :config {:linters {:invalid-arity {:level :off}}}
     ;; :cache takes a string, file or boolean
     :cache (io/file "/tmp/clj-kondo-cache")
     ;; only relevant when linting stdin
     :lang :clj})))
(first findings)
(clj-kondo/print-findings! res)

@simon-brooke
Copy link

That looks good: I can work with that. This is the clojure-api branch, yes?

@borkdude
Copy link
Member

borkdude commented Jun 4, 2019

@simon-brooke confirmed! I'll add some tests to it. if you have feedback on the API (naming, etc.) now is the time for change. 🙂

@simon-brooke
Copy link

I absolutely don't mind what keys you have, provided they're consistent and distinct. Rewriting keys in a map is trivial and not very expensive. What I mind is all the data is available, which in your proposed structure if seems to be.

@borkdude
Copy link
Member

borkdude commented Jun 5, 2019

small change: print-findings! was renamed to print!, also the 2 API functions now have docstrings.

borkdude added a commit that referenced this issue Jun 6, 2019
Implements feature request in #226
@borkdude
Copy link
Member

borkdude commented Jun 6, 2019

Fixed with bb80290

@borkdude borkdude closed this as completed Jun 7, 2019
borkdude added a commit that referenced this issue Jun 8, 2019
commit e394cb5
Author: Michiel Borkent <michielborkent@gmail.com>
Date:   Sat Jun 8 15:01:46 2019 +0200

    replace instead of merge config

commit 179fdcf
Author: Michiel Borkent <michielborkent@gmail.com>
Date:   Fri Jun 7 10:41:00 2019 +0200

    bump versions

commit cfaf526
Author: Michiel Borkent <michielborkent@gmail.com>
Date:   Fri Jun 7 10:19:49 2019 +0200

    v2019.06.07-alpha

commit 44d9ba1
Author: Michiel Borkent <michielborkent@gmail.com>
Date:   Thu Jun 6 21:40:16 2019 +0200

    fix false positive with associative destructuring

commit 627a22c
Author: Michiel Borkent <michielborkent@gmail.com>
Date:   Thu Jun 6 12:29:34 2019 +0200

    clojure API: files -> lint

commit bb80290
Author: Michiel Borkent <michielborkent@gmail.com>
Date:   Thu Jun 6 10:33:56 2019 +0200

    Clojure/JVM API

    Implements feature request in #226
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants