clj-commons / kibit Goto Github PK

Recommendation:
(update-in response [?key] json/generate-string)

It appears that [?key] is a placeholder that did not resolve correctly.

It would be awesome, if kibit could support the programmer with cleaning up (ns ...) declarations. By that I mean the following:

1. Suggest removing unused but :import-ed classes.
2. Suggest removing unused but :require-ed or :use-d namespaces.
3. Suggest removing unused vars in the :only clause of :use.
4. If a namespace is required with alias (e.g., (:require [foo.bar.baz :as fbb])), suggest replacing fully qualified var accesses like foo.bar.baz/some-var with fbb/some-var.
5. If a class is imported, suggest replacing fully qualified accesses with non-qualified ones.

Is that doable with kibit?

Given the code:

(fn [bar] [:foo bar])

Kibit will suggest replacing with

:foo

which is not equivalent.

It seems that kibit can't tell the two forms apart and is giving the suggestion for:

(fn [bar] (:foo bar))

This manifests itself most immediately right now in that e.g. code processed using cljx loses all whitespace, and therefore stack traces and such report incorrect line numbers. I can imagine more significant issues around using LineNumberingPushbackReader (e.g. not being able to apply rules to tagged reader literals, #= forms, and probably more), but the whitespace issue is what's biting me personally at the moment.

Something like a "lossless" reader would be more appropriate, with sjacket being the best candidate so far. (It's quite young itself, but is currently used by reply and therefore leiningen FWIW.)

(into [] ...) is not always equivalent to (vec ...), for example when the argument must be used within a reducer (a common case with core.reducers functions). In these cases, "into" works since it uses reduce internally, whereas "vec" doesn't.

Maybe this rule should be deactivated by default.

I was just wondering, is it possible to get kibit to do recursive rules? What I am thinking is something like this

(:c (:b (:a {:a {:b {:c 1}}})))
  => (get-in {:a {:b {:c 1}}} [:a :b :c])

This would require a recursive lookup to see if it makes sense to do the translation.

There probably are other rules, but this is the first one I could think of.

I have code similar to the following

(doto (SomeJavaClass.)
  (-> .getSomething (.setSomething "foo")))

Kibit is recommending that I change
(-> .getSomething (.setSomething "foo")))
to
(.setSomething .getSomething "foo")
which probably isn't correct.

Is there a reason why the simplification occurs in postwalk order instead of prewalk order?
If not, I'd prefer prewalk because then I can write rules to match forms based on their innards without worrying about those innards getting mangled by other rules.
This came up for me when trying to replace metadata'd forms with a keyword to mark them for removal:

(defn ^:cljs only-works-on-the-web [x] ...)

should become :remove-me.
Except that I also want to remove individual forms with that metadata:

^:cljs (toplevel-form)

(this case it doesn't matter if you prewalk or postwalk).
In the former case, though, if you postwalk, you end up with

(defn :remove-me [x] ...)

Somewhere along the way, the arithmetic rules stopped working:
user=> (kibit/check-form '(if (= 0 0) (println "a") (println "a")))
nil
user=> (kibit/check-form '(if (= 0 x) (println "a") (println "a")))
nil

The first step should be capturing these rules in tests and then working on the fix

I see this output from kibit:

[null] Consider:
  warn
instead of:
  #(warn %)

However taking kibit's advice results in the following compile error:

Can't take value of a macro: #'clojure.tools.logging/warn

I guess the unneeded functions section of misc.clj needs to understand the difference between a function invocation and a macro invocation.

Something like this would be great.
lein kibit :rules custom_rules.clj

This can help write custom rules for project specific things.

Unless there's some particular reason for the limitation. One benefit of this is that kibit can be run on Clojure itself without having to create a project.clj. The time it takes for kibit to run on Clojure's source is also a reasonable performance benchmark.

Hi there,
this is not a real "issue", but I thought it might be the best way to get in touch ;)

A while ago I wrote an Eclipse plugin during my diploma thesis called Cinder.

It parses an XML file that is generated (for us usually via several static code analysis tools for PHP) and the main benefit is that you get an easy overview of code smells/findings and more importantly, a doubleclick sends you directly to the offending line in the eclipse editor.

It all comes together as I'm using CCW for Clojure development.

To cut a long story short, this commit is a basic implementation of an xml-reporter and a "lein kibit_xml" task that generates a suitable XML file.

It's a proof of concept and I didn't bother to make pprint-code output fit in nicely and you still need to run
$ lein kibit_xml > kibit.xml

Looking at the format it's definitely not perfect, the "pattern" should hold the rule name, but I thought I'd ask for a little feedback before continuing really or doing a Pull Request (code definitely needs some cleanup) :)

kibit looks great so far, just the CLI output seems a little tedious to work with, so at least for Eclipse/CCW users this could make things easier.

Helpful for makefiles

I want to e.g. run lein kibit as part of a sanity check for builds.

Kibit makes suggestions that are valid most of the time but are not always exactly equivalent. #83 is a good example of this, where into [] and vec behave differently when used with core.reducers. I think it would be useful to be able to add comments to rules to explain these caveats.

There are also new Clojure forms that were introduced in 1.6. It would be good to check the version of Clojure the project is running on and only present rules that are valid on their version of Clojure.

For example, kibit suggests

(map Double/parseDouble...)

instead of

(map #(Double/parseDouble %)...)

I don't think there is a more idiomatic way to do this than the anonymous function wrapper. So kibit should not be suggesting any changes at all in this case. If there actually is a more idiomatic way, then kibit should be suggesting that since the current suggestion isn't valid clojure code.

Kibit produces output like the following:

Consider using:
  (update-in bar [?key] or (:spam foo))
instead of:
  (assoc bar :eggs (or (:eggs foo) (:spam foo)))

Would it be possible to not suggest update-in when the key is ?key?

For the s-expr (. Thread (sleep (read-string sleep-time))) Kibit 0.0.3 suggests (.clojure.lang.LazySeq@7bcd9a98 Thread)

Kibit suggests:

[177] Consider:
(when (all-streams-complete?)
((deliver @ALL-CLEAR :true) (info "All streams complete.")))

instead of:
(if (all-streams-complete?)
(do
(deliver @ALL-CLEAR :true)
(info "All streams complete.")))

This doesn't compile, on account of the extra parentheses it should be

(when (all-streams-complete?)
(deliver @ALL-CLEAR :true)
(info "All streams complete."))

Looks like Kibit got confused by this usage of ->:

;; [223] Consider:
  ((vary-meta assoc :midje/faked-function true)
    (fn [& actual-args]
      (handle-mocked-call the-var actual-args fn-fakes)))
;; instead of:
  (-> (fn [& actual-args]
        (handle-mocked-call the-var actual-args fn-fakes))
   (vary-meta assoc :midje/faked-function true))

Thanks for writing this library; quite useful.

I don't have time right now to dive into this, but I used kibit with lein2 and got the following ClassCastException on this code base:

Exception in thread "main" java.lang.ClassCastException: clojure.lang.Keyword cannot be cast to java.util.Map$Entry
    at clojure.lang.APersistentMap.cons(APersistentMap.java:41)
    at clojure.lang.RT.conj(RT.java:544)
    at clojure.core$conj.invoke(core.clj:83)
    at clojure.core.logic$eval1190$fn__1191.invoke(logic.clj:704)
    at clojure.core.logic$eval362$fn__363$G__353__370.invoke(logic.clj:37)
    at clojure.core.logic.Substitutions.walk_STAR_(logic.clj:181)
    at clojure.core.logic$eval1175$fn__1176$fn__1177.invoke(logic.clj:680)
    at clojure.core$map$fn__3811.invoke(core.clj:2432)
    at clojure.lang.LazySeq.sval(LazySeq.java:42)
    at clojure.lang.LazySeq.seq(LazySeq.java:60)
    at clojure.lang.Cons.next(Cons.java:39)
    at clojure.lang.LazySeq.next(LazySeq.java:92)
    at clojure.lang.RT.next(RT.java:580)
    at clojure.core$next.invoke(core.clj:64)
    at clojure.core.logic$eval1162$fn__1163.invoke(logic.clj:663)
    at clojure.core.logic$eval334$fn__335$G__325__342.invoke(logic.clj:34)
    at clojure.core.logic.Substitutions._reify_STAR_(logic.clj:197)
    at clojure.core.logic$eval1162$fn__1163.invoke(logic.clj:663)
    at clojure.core.logic$eval334$fn__335$G__325__342.invoke(logic.clj:34)
    at clojure.core.logic.Substitutions._reify_STAR_(logic.clj:197)
    at clojure.core.logic$eval1162$fn__1163.invoke(logic.clj:663)
    at clojure.core.logic$eval334$fn__335$G__325__342.invoke(logic.clj:34)
    at clojure.core.logic.Substitutions._reify_STAR_(logic.clj:197)
    at clojure.core.logic$eval1162$fn__1163.invoke(logic.clj:663)
    at clojure.core.logic$eval334$fn__335$G__325__342.invoke(logic.clj:34)
    at clojure.core.logic.Substitutions._reify_STAR_(logic.clj:197)
    at clojure.core.logic.Substitutions._reify(logic.clj:201)
    at jonase.kibit.core$unify$fn__2381$fn__2382$_inc__2383$fn__2392.invoke(core.clj:48)
    at clojure.core.logic.Substitutions.bind(logic.clj:208)
    at jonase.kibit.core$unify$fn__2381$fn__2382$_inc__2383.invoke(core.clj:48)
    at clojure.core.logic$eval1268$fn__1269$fn__1270.invoke(logic.clj:813)
    at clojure.lang.LazySeq.sval(LazySeq.java:42)
    at clojure.lang.LazySeq.seq(LazySeq.java:67)
    at clojure.lang.RT.seq(RT.java:466)
    at clojure.core$seq.invoke(core.clj:133)
    at clojure.core$dorun.invoke(core.clj:2723)
    at clojure.core$doall.invoke(core.clj:2739)
    at jonase.kibit.core$unify.invoke(core.clj:48)
    at jonase.kibit.core$check_form$fn__2400.invoke(core.clj:71)
    at clojure.core$some.invoke(core.clj:2388)
    at jonase.kibit.core$check_form.invoke(core.clj:71)
    at jonase.kibit.core$check_form.invoke(core.clj:68)
    at clojure.core$keep$fn__5766.invoke(core.clj:6351)
    at clojure.lang.LazySeq.sval(LazySeq.java:42)
    at clojure.lang.LazySeq.seq(LazySeq.java:67)
    at clojure.lang.RT.seq(RT.java:466)
    at clojure.core$seq.invoke(core.clj:133)
    at leiningen.kibit$kibit$fn__2431.invoke(kibit.clj:16)
    at leiningen.kibit$kibit.invoke(kibit.clj:15)
    at clojure.lang.Var.invoke(Var.java:401)
    at clojure.lang.AFn.applyToHelper(AFn.java:161)
    at clojure.lang.Var.applyTo(Var.java:518)
    at clojure.core$apply.invoke(core.clj:602)
    at leiningen.core.main$resolve_task$fn__648.doInvoke(main.clj:54)
    at clojure.lang.RestFn.invoke(RestFn.java:410)
    at clojure.lang.AFn.applyToHelper(AFn.java:161)
    at clojure.lang.RestFn.applyTo(RestFn.java:132)
    at clojure.lang.AFunction$1.doInvoke(AFunction.java:29)
    at clojure.lang.RestFn.applyTo(RestFn.java:137)
    at clojure.core$apply.invoke(core.clj:602)
    at leiningen.core.main$apply_task.invoke(main.clj:75)
    at leiningen.core.main$_main.doInvoke(main.clj:124)
    at clojure.lang.RestFn.invoke(RestFn.java:408)
    at clojure.lang.Var.invoke(Var.java:401)
    at clojure.lang.AFn.applyToHelper(AFn.java:161)
    at clojure.lang.Var.applyTo(Var.java:518)
    at clojure.core$apply.invoke(core.clj:600)
    at clojure.main$main_opt.invoke(main.clj:323)
    at clojure.main$main.doInvoke(main.clj:426)
    at clojure.lang.RestFn.invoke(RestFn.java:436)
    at clojure.lang.Var.invoke(Var.java:409)
    at clojure.lang.AFn.applyToHelper(AFn.java:167)
    at clojure.lang.Var.applyTo(Var.java:518)
    at clojure.main.main(main.java:37)

@bbatsov has a nice written Clojure style guidelines repo and there is the default Clojure guidelines that cover many aspects that could be seen as generalized/community accepted, and being particularly seen as a company's flavour or "this is the only way to do it".

If that is reasonable and feasible, I think it is worth going through the guidelines, make them list items, and partition/rank them in relevance and implement new rules that do not exist yet in kibit.

The icing on the cake would be to somehow (maybe just links to the clj filename + lines) reference the guideline to the rule in the code.

I can help out if you see a point in doing this. Maybe 90% of the rules mentioned there are already covered (I have to look through the kibit source code again, you might have a better mental overview what's covered and what not, maybe for a good reason).

Edited to show list in first post

• Use seq as a terminating condition to test whether a sequence is empty (this technique is sometimes called nil punning).
• Prefer vec over into when you need to convert a sequence into a vector.
• Use if-let instead of let + if - #54
• Use when-let instead of let + when - #54
• Use if-not instead of (if (not ...) ...)
• Use when-not instead of (when (not ...) ...)
• Use when-not instead of (if-not ... (do ...).
• Use not= instead of (not (= ...)). - kibit.rules.equality
• When doing comparisons, keep in mind that Clojure's functions <, >, etc. accept a variable number of arguments.
• Prefer % over %1 in function literals with only one parameter.
• Prefer %1 over % in function literals with more than one parameter.
• Don't wrap functions in anonymous functions when you don't need to.
• Don't use function literals if the function body will consist of more than one form.
• Favor the use of complement versus the use of an anonymous function.
• Leverage comp when it would yield simpler code.
• Leverage partial when it would yield simpler code.
• Prefer the use of the threading macros -> (thread-first) and ->> (thread-last) to heavy form nesting.
• Prefer .. to -> when chaining method calls in Java interop.
• Use :else as the catch-all test expression in cond
• Prefer condp instead of cond when the predicate & expression don't change.
• Prefer case instead of cond or condp when test expressions are compile-time constants.
• Use short forms in cond and related. If not possible give visual hints for the pairwise grouping with comments or empty lines.
• Use a set as a predicate when appropriate.
• Use (inc x) & (dec x) instead of (+ x 1) and (- x 1).
• Use (pos? x), (neg? x) & (zero? x) instead of (> x 0), (< x 0) & (= x 0).
• Use list* instead of a series of nested cons invocations.
• Use the sugared Java interop forms.
• Use the compact metadata notation for metadata that contains only slots whose keys are keywords and whose value is boolean true.
• Denote private parts of your code. - not able to determine this statically
• Be careful regarding what exactly do you attach metadata to. - eastwood maybe
• Use _ for destructuring targets and formal arguments names whose value will be ignored by the code at hand. - eastwood
• Follow clojure.core's example for idiomatic names like pred and coll. - not able to determine this statically
• Prefer the use of keywords for hash keys. - stylistic, may be cases where string keys are required
• Prefer the use of the literal collection syntax where applicable. However, when defining sets, only use literal syntax when the values are compile-time constants.
• Avoid accessing collection members by index whenever possible.
• Prefer the use of keywords as functions for retrieving values from maps, where applicable.
• Leverage the fact that most collections are functions of their elements.
• Leverage the fact that keywords can be used as functions of a collection.
• Avoid the use of ref-set whenever possible. - eastwood
• Try to use swap! rather than reset!, where possible. - not possible to suggest an equivalent swap! form given a reset! form
• Prefer string manipulation functions from clojure.string over Java interop or rolling your own. - .toUpperCase, .toLowerCase, and .trim are candidates for this.
• Reuse existing exception types. Idiomatic Clojure code — when it does throw an exception — throws an exception of a standard type - eastwood
• Favor with-open over finally

Running lein2 kibit gives the following suggestion:

[212] Consider:
  ((assoc :body (FileEntity. body (or body-encoding "UTF-8"))) req)
instead of:
  (-> req (assoc :body (FileEntity. body (or body-encoding "UTF-8"))))

However, the suggested alternative is not correct, it should be:

(assoc req :body (FileEntity. body (or body-encoding "UTF-8")))

I always have a repl of my project open so instead of launching a lein kibit which uses precious memory I'd like to just run a few repl commands. I tried to figure this out on my own, but I ran into a roadblock.

While trying to require kibit.driver I get the following error:

=> (require 'kibit.check)
CompilerException java.lang.RuntimeException: Unable to resolve var: *default-data-reader-fn* in this context, compiling:(kibit/check.clj:204)

Did I approach this the wrong way or is this not a feature?

Kibit suggested:

Consider using:
Color.
instead of:
#(Color. %)

but this is not allowed

It recommends that I change
clojure (map #(String. %) ["a" "b"])
to
clojure (map String. ["a" "b"])
which is invalid afaik

Just curious what the state of the unit tests is expected to be. A fresh checkout and "lein test" gives me 19 failures on the master branch.

Exception in thread "main" java.lang.StackOverflowError
at clojure.core.logic$tree_term_QMARK_.invoke(logic.clj:972)
at clojure.core.logic$walk_STAR_$fn__1462.invoke(logic.clj:419)
at clojure.core.logic$eval1645$fn__1646.invoke(logic.clj:1068)
at clojure.core.logic$eval375$fn__376$G__366__383.invoke(logic.clj:85)
at clojure.core.logic$walk_STAR_.invoke(logic.clj:416)
at clojure.core.logic$walk_STAR_$fn__1462.invoke(logic.clj:420)
at clojure.core.logic$eval1645$fn__1646.invoke(logic.clj:1068)
at clojure.core.logic$eval375$fn__376$G__366__383.invoke(logic.clj:85)
at clojure.core.logic$walk_STAR_.invoke(logic.clj:416)
at clojure.core.logic$walk_STAR_$fn__1462.invoke(logic.clj:420)
at clojure.core.logic$eval1645$fn__1646.invoke(logic.clj:1068)
at clojure.core.logic$eval375$fn__376$G__366__383.invoke(logic.clj:85)

Could suggest:
io/file instead of (File.)
Use of stream functions instead of Java streams
io/reader and io/writer

these rules from misc.clj:

  ;; filter
  [(filter (complement ?pred) ?coll) (remove ?pred ?coll)]
  [(filter #(not (?pred ?x)) ?coll) (remove ?pred ?coll)]

give the correct suggestion for
(filter (complement zero?) [0 1 2 3 4 0 1 2 3 4])

but not for
(filter #(not (zero? %)) [0 1 2 3 4 0 1 2 3 4])

For this code:

(mapcat (fn [from] (map (fn [to] [from to]) (get gmap from))) nodes)

Kibit makes the following suggestion:

Consider using:
  from
instead of:
  (fn [to] [from to])

This is due to, as I see it, [from to] being confused with (from to).

(take n (repeatedly f)) can be simplified to (repeatedly n f).

Certainly possible this is an issue with my environment, but I'm getting errors with Kibit parsing 0N. My project uses Clojure 1.3.0.

Here's the error:

➜  ~knockbox git:(master) lein plugin install jonase/kibit 0.0.2
[INFO] Unable to find resource 'jonase:kibit:jar:0.0.2' in repository central (http://repo1.maven.org/maven2)
Copying 4 files to /var/folders/yq/5r69gv8d0vz99cptvsj3bqvm0000gn/T/lein-f03139ec-689d-4f01-aa79-204128b507cb/lib
Including kibit-0.0.2.jar
Including clojure-1.3.0.jar
Including core.logic-0.6.7.jar
Including java.classpath-0.1.1.jar
Including tools.namespace-0.1.2.jar
Created jonase-kibit-0.0.2.jar
➜  ~knockbox git:(master) lein kibit
Reflection warning, clojure/core/logic.clj:1491 - call to invoke can't be resolved.
Reflection warning, clojure/core/logic.clj:1491 - call to invoke can't be resolved.
Reflection warning, clojure/core/logic.clj:1491 - call to invoke can't be resolved.
Reflection warning, clojure/core/logic.clj:1491 - call to invoke can't be resolved.
Reflection warning, clojure/core/logic.clj:1491 - call to invoke can't be resolved.
Reflection warning, clojure/core/logic.clj:1491 - call to invoke can't be resolved.
Reflection warning, clojure/core/logic.clj:1491 - call to invoke can't be resolved.
Reflection warning, clojure/core/logic.clj:1491 - call to invoke can't be resolved.
Reflection warning, clojure/core/logic.clj:1491 - call to invoke can't be resolved.
Reflection warning, clojure/core/logic.clj:1491 - call to invoke can't be resolved.
Reflection warning, clojure/core/logic.clj:1491 - call to invoke can't be resolved.
Reflection warning, clojure/core/logic.clj:1491 - call to invoke can't be resolved.
Reflection warning, clojure/core/logic.clj:1491 - call to invoke can't be resolved.
Reflection warning, clojure/core/logic.clj:1491 - call to invoke can't be resolved.
Reflection warning, clojure/core/logic.clj:1491 - call to invoke can't be resolved.
Reflection warning, clojure/core/logic.clj:1491 - call to invoke can't be resolved.
Reflection warning, clojure/core/logic.clj:1491 - call to invoke can't be resolved.
Reflection warning, clojure/core/logic.clj:1491 - call to invoke can't be resolved.
Reflection warning, clojure/core/logic.clj:1491 - call to invoke can't be resolved.
Reflection warning, clojure/core/logic.clj:1491 - call to invoke can't be resolved.
== knockbox.core ==
== knockbox.internal ==
Exception in thread "main" java.lang.RuntimeException: java.lang.RuntimeException: clojure.lang.LispReader$ReaderException: java.lang.NumberFormatException: Invalid number: 0N (NO_SOURCE_FILE:0)
    at clojure.lang.Compiler.eval(Compiler.java:5440)
    at clojure.lang.Compiler.eval(Compiler.java:5391)
    at clojure.core$eval.invoke(core.clj:2382)
    at clojure.main$eval_opt.invoke(main.clj:235)
    at clojure.main$initialize.invoke(main.clj:254)
    at clojure.main$script_opt.invoke(main.clj:270)
    at clojure.main$main.doInvoke(main.clj:354)
    at clojure.lang.RestFn.invoke(RestFn.java:457)
    at clojure.lang.Var.invoke(Var.java:377)
    at clojure.lang.AFn.applyToHelper(AFn.java:172)
    at clojure.lang.Var.applyTo(Var.java:482)
    at clojure.main.main(main.java:37)
Caused by: java.lang.RuntimeException: java.lang.RuntimeException: clojure.lang.LispReader$ReaderException: java.lang.NumberFormatException: Invalid number: 0N
    at clojure.lang.LazySeq.sval(LazySeq.java:47)
    at clojure.lang.LazySeq.seq(LazySeq.java:56)
    at clojure.lang.Cons.next(Cons.java:39)
    at clojure.lang.RT.boundedLength(RT.java:1186)
    at clojure.lang.RestFn.applyTo(RestFn.java:130)
    at clojure.core$apply.invoke(core.clj:540)
    at clojure.core$mapcat.doInvoke(core.clj:2122)
    at clojure.lang.RestFn.invoke(RestFn.java:423)
    at jonase.kibit.core$check_file.invoke(core.clj:131)
    at jonase.kibit.core$check_file.invoke(core.clj:128)
    at leiningen.kibit$kibit$fn__2472.invoke(kibit.clj:16)
    at leiningen.kibit$kibit.invoke(kibit.clj:15)
    at clojure.lang.Var.invoke(Var.java:365)
    at clojure.lang.AFn.applyToHelper(AFn.java:161)
    at clojure.lang.Var.applyTo(Var.java:482)
    at clojure.core$apply.invoke(core.clj:542)
    at leiningen.core$apply_task.invoke(core.clj:259)
    at leiningen.core$_main.doInvoke(core.clj:325)
    at clojure.lang.RestFn.invoke(RestFn.java:410)
    at clojure.lang.AFn.applyToHelper(AFn.java:161)
    at clojure.lang.RestFn.applyTo(RestFn.java:132)
    at clojure.core$apply.invoke(core.clj:542)
    at leiningen.core$_main.invoke(core.clj:328)
    at user$eval42.invoke(NO_SOURCE_FILE:1)
    at clojure.lang.Compiler.eval(Compiler.java:5424)
    ... 11 more
Caused by: java.lang.RuntimeException: clojure.lang.LispReader$ReaderException: java.lang.NumberFormatException: Invalid number: 0N
    at clojure.lang.LazySeq.sval(LazySeq.java:47)
    at clojure.lang.LazySeq.seq(LazySeq.java:56)
    at clojure.lang.RT.seq(RT.java:450)
    at clojure.core$seq.invoke(core.clj:122)
    at clojure.core$map$fn__3699.invoke(core.clj:2088)
    at clojure.lang.LazySeq.sval(LazySeq.java:42)
    ... 35 more
Caused by: clojure.lang.LispReader$ReaderException: java.lang.NumberFormatException: Invalid number: 0N
    at clojure.lang.LispReader.read(LispReader.java:180)
    at clojure.core$read.invoke(core.clj:2884)
    at clojure.core$read.invoke(core.clj:2882)
    at jonase.kibit.core$read_ns$fn__2450.invoke(core.clj:95)
    at clojure.lang.LazySeq.sval(LazySeq.java:42)
    ... 40 more
Caused by: java.lang.NumberFormatException: Invalid number: 0N
    at clojure.lang.LispReader.readNumber(LispReader.java:218)
    at clojure.lang.LispReader.read(LispReader.java:136)
    at clojure.lang.LispReader.readDelimitedList(LispReader.java:1060)
    at clojure.lang.LispReader$ListReader.invoke(LispReader.java:900)
    at clojure.lang.LispReader.readDelimitedList(LispReader.java:1051)
    at clojure.lang.LispReader$ListReader.invoke(LispReader.java:900)
    at clojure.lang.LispReader.read(LispReader.java:145)
    ... 44 more

Check this question: http://stackoverflow.com/questions/10358149/in-clojure-1-4-what-is-the-use-of-refer-within-require
:use should be avoided completely in favor of :require.

Hi, is there a way to have lein kibit starting up once and then feed it one file after the other repeatedly? The start-up time has really been annoying me. Having to run lein kibit "path/file" repeatedly takes a lot of time. I'd rather have lein kibit start up once and then point it to files while it's running.

Right now Riemann's codebase gets suggestions like this:

== riemann.logging ==
[21] Consider:
  (.clojure.lang.LazySeq@fe724234 (Logger/getLogger logger))
instead of:
  (. (Logger/getLogger logger) (setLevel level))

[44] Consider:
  (.clojure.lang.LazySeq@7b82a37c (Logger/getRootLogger))
instead of:
  (. (Logger/getRootLogger) (setLevel Level/INFO))

Those obviously won't work.

I have a "sanity-check" alias in my Leiningen project.clj file that AOT-compiles the entire application's sources as an easy way just to make sure there is no unloadable code in place. Within the same project, then, these two commands in succession:

$ lein2 with-profile sanity-check do clean, compile
...
...
$ lein2 repl

…produces this exception:

Caused by: java.lang.IllegalArgumentException: No matching ctor found for class kibit.rules.util$compile_rule$fn__6921
    at clojure.lang.Reflector.invokeConstructor(Reflector.java:163)
    at clojure.lang.LispReader$EvalReader.invoke(LispReader.java:1036)
    at clojure.lang.LispReader$DispatchReader.invoke(LispReader.java:621)
    at clojure.lang.LispReader.read(LispReader.java:180)
    at clojure.lang.RT.readString(RT.java:1713)
    at kibit.rules.arithmetic__init.__init0(Unknown Source)
    at kibit.rules.arithmetic__init.<clinit>(Unknown Source)
    ... 117 more

A minor issue to be sure, but it's odd enough that I figured I'd file it; I don't think I've ever seen a circumstance where code won't load in the presence in AOT-compiled classfiles produced from that same code.

If the code contains namespace qualified keywords with aliases, Kibit errors out with a Invalid token exception.

The following code demonstrates the problem -

;;; foo.clj
(ns foo)

;;; bar.clj
(ns bar
  (:require [foo :as f]))

(defn xxx []
    ::f/some-kwd)

I think the problem is not in Kibit itself but the way it's invoking the Clojure compiler.

The readme says that if it finds the code (if (some test) (some action) nil) it will suggest replacing it with while. I think it means when, but this isn't right; single-branched if forms are totally acceptable if the focus is on the return value. Since when has an implicit do form, it is a way of signaling to readers that side-effects are involved, while if forms are all about what value is returned. Instead consider adding a rule for replacing (if cond (do some action)) with when.

e.g. ::foo always is always read as :user/foo, even if it is within the scope of another namespace.

This is related to gh-14 insofar as reading these keywords as expected requires support of the runtime; however, unlike that issue, we shouldn't need to load any code to obtain correct results. We just need to know when in the load process *ns* would change. I've been experimenting with this reworking of kibit.check/read-file with good results:

(defn read-file
     "Generate a lazy sequence of top level forms from a
   LineNumberingPushbackReader"
     [^LineNumberingPushbackReader r]
     (let [ns (atom *ns*)
           do-read (fn do-read []
                     (lazy-seq
                       (let [form (binding [*ns* @ns]
                                    (read r false eof))
                             [ns? new-ns k] (when (sequential? form) form)]
                         (when (and (symbol? new-ns)
                                 (or (= ns? 'ns) (= ns? 'in-ns)))
                           (reset! ns (create-ns new-ns)))
                         (when-not (= form eof)
                           (cons form (do-read))))))]
       (do-read)))

This works for the check-reader and check-file cases when an ns form is present, but will fail on check-expr unless the expression in question is from 'user. To be complete, all three should probably accept a new :ns option to specify what the initial reading namespace should be.

Thoughts?

When I run lein kibit on one of the Lein plugins (https://github.com/kumarshantanu/lein-localrepo), I get the following exception trace:

Exception in thread "main" java.lang.RuntimeException: java.lang.RuntimeException: java.lang.RuntimeException: java.lang.ClassCastException: java.lang.Character cannot be cast to java.util.Map$Entry (NO_SOURCE_FILE:0)
    at clojure.lang.Compiler.eval(Compiler.java:5440)
    at clojure.lang.Compiler.eval(Compiler.java:5391)
    at clojure.core$eval.invoke(core.clj:2382)
    at clojure.main$eval_opt.invoke(main.clj:235)
    at clojure.main$initialize.invoke(main.clj:254)
    at clojure.main$script_opt.invoke(main.clj:270)
    at clojure.main$main.doInvoke(main.clj:354)
    at clojure.lang.RestFn.invoke(RestFn.java:457)
    at clojure.lang.Var.invoke(Var.java:377)
    at clojure.lang.AFn.applyToHelper(AFn.java:172)
    at clojure.lang.Var.applyTo(Var.java:482)
    at clojure.main.main(main.java:37)
Caused by: java.lang.RuntimeException: java.lang.RuntimeException: java.lang.RuntimeException: java.lang.ClassCastException: java.lang.Character cannot be cast to java.util.Map$Entry
    at clojure.lang.LazySeq.sval(LazySeq.java:47)
    at clojure.lang.LazySeq.seq(LazySeq.java:63)
    at clojure.lang.RT.seq(RT.java:450)
    at clojure.core$seq.invoke(core.clj:122)
    at leiningen.kibit$kibit$fn__2472.invoke(kibit.clj:16)
    at leiningen.kibit$kibit.invoke(kibit.clj:15)
    at clojure.lang.Var.invoke(Var.java:365)
    at clojure.lang.AFn.applyToHelper(AFn.java:161)
    at clojure.lang.Var.applyTo(Var.java:482)
    at clojure.core$apply.invoke(core.clj:542)
    at leiningen.core$apply_task.invoke(core.clj:264)
    at leiningen.core$_main.doInvoke(core.clj:331)
    at clojure.lang.RestFn.invoke(RestFn.java:410)
    at clojure.lang.AFn.applyToHelper(AFn.java:161)
    at clojure.lang.RestFn.applyTo(RestFn.java:132)
    at clojure.core$apply.invoke(core.clj:542)
    at leiningen.core$_main.invoke(core.clj:334)
    at user$eval42.invoke(NO_SOURCE_FILE:1)
    at clojure.lang.Compiler.eval(Compiler.java:5424)
    ... 11 more
Caused by: java.lang.RuntimeException: java.lang.RuntimeException: java.lang.ClassCastException: java.lang.Character cannot be cast to java.util.Map$Entry
    at clojure.lang.LazySeq.sval(LazySeq.java:47)
    at clojure.lang.LazySeq.seq(LazySeq.java:63)
    at clojure.lang.RT.seq(RT.java:450)
    at clojure.core$seq.invoke(core.clj:122)
    at clojure.core$dorun.invoke(core.clj:2450)
    at clojure.core$doall.invoke(core.clj:2465)
    at jonase.kibit.core$unify.invoke(core.clj:48)
    at jonase.kibit.core$check_form$fn__2441.invoke(core.clj:71)
    at clojure.core$some.invoke(core.clj:2053)
    at jonase.kibit.core$check_form.invoke(core.clj:71)
    at jonase.kibit.core$check_form.invoke(core.clj:68)
    at clojure.core$keep$fn__5589.invoke(core.clj:5665)
    at clojure.lang.LazySeq.sval(LazySeq.java:42)
    ... 29 more
Caused by: java.lang.RuntimeException: java.lang.ClassCastException: java.lang.Character cannot be cast to java.util.Map$Entry
    at clojure.lang.LazySeq.sval(LazySeq.java:47)
    at clojure.lang.LazySeq.seq(LazySeq.java:56)
    at clojure.lang.Cons.next(Cons.java:39)
    at clojure.lang.LazySeq.next(LazySeq.java:88)
    at clojure.lang.RT.next(RT.java:560)
    at clojure.core$next.invoke(core.clj:61)
    at clojure.core.logic$eval1213$fn__1214.invoke(logic.clj:663)
    at clojure.core.logic$eval395$fn__396$G__386__403.invoke(logic.clj:34)
    at clojure.core.logic.Substitutions._reify_STAR_(logic.clj:197)
    at clojure.core.logic$eval1213$fn__1214.invoke(logic.clj:663)
    at clojure.core.logic$eval395$fn__396$G__386__403.invoke(logic.clj:34)
    at clojure.core.logic.Substitutions._reify_STAR_(logic.clj:197)
    at clojure.core.logic.Substitutions._reify(logic.clj:201)
    at jonase.kibit.core$unify$fn__2422$fn__2423$_inc__2424$fn__2433.invoke(core.clj:48)
    at clojure.core.logic.Substitutions.bind(logic.clj:208)
    at jonase.kibit.core$unify$fn__2422$fn__2423$_inc__2424.invoke(core.clj:48)
    at clojure.core.logic$eval1317$fn__1318$fn__1319.invoke(logic.clj:813)
    at clojure.lang.LazySeq.sval(LazySeq.java:42)
    ... 41 more
Caused by: java.lang.ClassCastException: java.lang.Character cannot be cast to java.util.Map$Entry
    at clojure.lang.APersistentMap.cons(APersistentMap.java:41)
    at clojure.lang.RT.conj(RT.java:524)
    at clojure.core$conj.invoke(core.clj:78)
    at clojure.core.logic$eval1241$fn__1242.invoke(logic.clj:704)
    at clojure.core.logic$eval423$fn__424$G__414__431.invoke(logic.clj:37)
    at clojure.core.logic.Substitutions.walk_STAR_(logic.clj:181)
    at clojure.core.logic$eval1232$fn__1233.invoke(logic.clj:687)
    at clojure.core.logic$eval423$fn__424$G__414__431.invoke(logic.clj:37)
    at clojure.core.logic.Substitutions.walk_STAR_(logic.clj:181)
    at clojure.core.logic$eval1226$fn__1227$fn__1228.invoke(logic.clj:680)
    at clojure.core$map$fn__3699.invoke(core.clj:2096)
    at clojure.lang.LazySeq.sval(LazySeq.java:42)
    ... 58 more

kibit 0.0.1 found some spots in my code that could really be improved. With version 0.0.2, it spots some more, but not all of it's suggestions are correct, so probably some rules need tweaking.

Consider .getEClassifiers instead of (fn [ep] (.getEClassifiers ep))

That would be a good suggestion, but my function is actually (fn [^EPackage ep] (.getEClassifiers ep)) used for mapcat-ing a large seq. So the actual :tag metadata is the reason for wrapping the java call in an explicit function.

Consider (id g) instead of (fn [g] [(id g) g])

Now that's actually a kibit bug. The suggestion in clearly not equivalent to the function. The complete form is (map (fn [g] [(id g) g]) gs)).

[null] Consider .getSecond instead of (fn* [p1__342923#] (.getSecond p1__342923#))

Same as the first one, the reason for the explicit function is :tag metadata: (map #(.getSecond ^PermutationEntry %) (seq (.getTopologicalOrder a)).

I had the following pattern in an old project:

(if (not ?x) nil ?y)

kibit suggested the following alternative:

(when-not (not ?x) ?y)

This could be simplified to:

(when ?x ?y)

Therefore, I believe the following rule should be added:

[(when-not (not ?x) . ?y) (when ?x . ?y)]

Hope this helps!

Hi jonase,

I am loving kibit; it is finding lots of non-idiomatic code in my projects.

A lot of my code-base is ClojureScript and kibit works just as well on that as you would expect. Problem is, it doesn't check cljs files by default. I wrote a little helper to run kibit one file at a time in my project as a work around.

If you plonk this into driver.clj:

(defn find-clojure-sources-in-dir [dir]
  (letfn [(clojure-source-file? [file]
            (and (.isFile file)
                 (let [name (.getName file)]
                   (or (.endsWith name ".clj")
                       (.endsWith name ".cljs")))))]

    (sort-by #(.getAbsolutePath %)
             (filter clojure-source-file? (file-seq dir)))))

:this is just a modified version of the same function in clojure.tools.namespace.

I would submit a pull-request but I am not smart enough to get a locally built plugin to work in a test project. (Maven hates me, and everyone who looks like me). I don't want to submit a pull-request that isn't tested, but I have used the above function in my work-around script and it works a treat.

By the way, if you know of a tutorial on building and testing a Leiningen 2 plugin, I will get this working properly and submit you a proper pull-request.

Thanks for building a great tool!

I've run kibit 0.0.4 on my project, and it suggested the following (several occurences of the same pattern).

[100] Consider:
  (doto (.getGraphClass (schema g))
    (.createVertexClass (name qname))
    (.setAbstract (boolean abstract)))
instead of:
  (-> (.getGraphClass (schema g))
   (doto (.createVertexClass (name qname))
     (.setAbstract (boolean abstract))))

But the suggested form is not equivalent to my original form. In my form, setAbstract is invoked on the value returned by the call to createVertexClass, whereas the suggestion invokes it on the value of getGraphClass.

One possible approach:

(def rules
  (map prep
       '[[(fn ?args (?fun . ?args)) :when [fn-call?] :alt ?fun]]))

(defne guardso [rule guards]
  ([[_ :when guards . _] _])
  ([[_ :alt _] ()]))

(defne alto [rule alt]
  ([[_ :alt alt] _])
  ([[_ _ _ :alt alt] _]))

(defn not-method? [sym]
  (not= (first (str sym)) \.))

(defne fn-call? [expr]
  ([['fn [_ . _] [fun . _]]]
     (project [fun]
       (pred fun symbol?)
       (pred fun not-method?))))

(defn ->guard [name]
  (case name
    'fn-call? fn-call?))

(defne check-guards [expr guards]
  ([_ ()])
  ([_ [name . rest]]
     (project [name]
       ((->guard name) expr))
     (check-guards expr rest)))

(defn simplify [expr rules]
  (run* [q]
    (fresh [rule pat guards alt]
      (membero rule rules)
      (firsto rule pat)
      (== pat expr)
      (guardso rule guards)
      (check-guards expr guards)
      (alto rule alt)
      (== q alt))))

(comment
  (simplify '(fn [x] (inc x)) rules)     ;; (inc)
  (simplify '(fn [x] (.method x)) rules) ;; ()
  )

kibit fails checking namespaces when using a non-standard :source-path.

The problem lies in kibit.core/source-file, it doesn't prepend leiningen's :source-path to the path.

The difference is that ‘(zero? x)‘ fails with exception if x is ‘nil‘. Perhaps some warning should be added, as by following the suggestion from kibit it is possible to introduce a subtle bug (like I recently did).

Hi, Just ran 0.0.2 against Midje's code base, and received this message:

[64] Consider (when-not (instance? Throwable throwable) ((throw (user-error "Right side of =throws=> should extend Throwable."))))

instead of 
(when (not (instance? Throwable throwable)) (throw (user-error "Right side of =throws=> should extend Throwable.")))

Looks good, except ((throw ... has one too many layers of parenthesis.

Where as the below works just fine:

(when-not (instance? Throwable throwable) 
      (throw (user-error "Right side of =throws=> should extend Throwable.")))