Clojurians Log v2

I just imagined someone putting error/log message text into source code that then goes through a demangling process like rot13 before being printed, in a huge code base with many developers, and shuddered. Someone would have to be pretty wicked to do that.

haha

if I have

(ns foo.bar
  (:require [baz.baz :as b]))

(def x {::b/baz 42})

hang on, I’ll check

Cannot reproduce locally, given:

(ns baz.core
  (:require [baz.non-core :as b])
  (:gen-class))

(def b  {::b/id 'a})
(defn -main
  "I don't do a whole lot ... yet."
  [& args]
  (println "Hello, World! " b))

and

(ns baz.non-core)

(def lol 'lol)

Could you create a minimal repro on github for me to see if I can repro as well?

hmm.. trying to make a minimal repro right now - this namespace is super small so I think I'll start with it, and pare it down...

yeah, the smallest example doesn't fail so this must be some corner case I'm hitting

I think Clojure 1.9 calls that an invalid keyword?

and Eastwood uses not Clojure's reader, but a version of the tools.reader library that has been updated to be compatible with Clojure 1.9 features like namespaced keywords in maps.

it is valid, and the simple version does accept it

there's some corner case rejecting it

I have a half-recollection that :b/baz is the correct syntax according to Clojure 1.9, and it rejects ::b/baz. Let me check.

:b/baz is a literal, ::b/baz auto-expands to whatever b is aliased to

I use ::b/baz is as @noisesmith describes.

OK, my half-recollection is incorrect, then.

OK, the change I was half-remembering is that it used to be Clojure and tools.reader accepted ::anythinghere/kw, but then changed so that anythinghere was required to be an alias.

fascinating

The case you have hit sounds like it could be a bug in tools.reader, or more likely in how Eastwood is using it.

:thumbsup: - I'll let you know if I come up with a repro that makes sense outside this complex project

Is the complex project using Clojure 1.9?

yeah

ugh - it's not a thing I can disable in config since it's tools.reader blowing up

If so, then I would guess that unless it has a bug in its decision of whether the alias is a valid alias or not (seems unlikely), then it is an actual valid alias, hopefully one obviously in the ns form

you can disable linting the entire namespace in Eastwood options.

the alias is definitely in the ns form - I'll do that for now, thanks

@andy.fingerhut while you’re here.

Do you have much knowledge of the different callbacks that are set up and passed around in opts?

Specifically, are they part of the public api of Eastwood, or is it more something that’s used by devs when developing Eastwood?

I am pretty sure I implemented those, a few years back. I don't recall those being documented in the README anywhere. If anyone relies on them, they haven't asked me about them.

The most likely people to have ever looked at and/or taken advantage of such things would be IDE/editor integrations with Eastwood.

The 'print to stdout' and 'return a sequence of maps' APIs are documented in the README, and would be good to preserve their APIs

:callback - a default message callback function, which simply formats all callback data as strings and prints it to *out*, or the writer specified by the value of the :out key in the options map (e.g. if it is a string, the file named by that string will be written).

By those, I mean the eastwood.lint/eastwood and eastwood.lint/lint functions.

my issue goes away when I eliminate a completely different error (calling defmethod on something that isn't a multimethod), which is deeper in recursive loads of loading the ns that had the alias that erroneously errored

README has said this for a long time: "There is no documentation for this callback function yet. You are welcome to read Eastwood source code to see examples of how to write one, but note that this is alpha-status code that will likely have API changes in future Eastwood versions."

Nice!

I left us some freedom there 🙂

So basically, as long as the public api of eastwood.lint/eastwood and eastwood.lint/lint is preserved we’re good?

Well, have you heard of Hyrum's Law? Despite that, I have no reason to believe that very many people would be affected by changing the callback parts of the implementation.

(if any would)

The reason I’d like to change it is that (at least for me) it’s kind of hard to follow.

Changing those APIs would seem very break-y to me, yes. Keeping them I expect will keep 95% or more of Eastwood users content, if not 100%

in terms of what functions are actually called (at the call site)

Understood. I do not consider it a shining example of code I would suggest for a future edition of the book "Beautiful Code" 🙂

lol 🙂

(defn report-warnings [cb warning-count warnings]
  (swap! warning-count + (count warnings))
  (doseq [warning warnings]
    (cb warnings)))

“What is cb at this point?” is a hard question to answer 🙂

But I don’t think I have a better solution of the top of my head 😞

Yes, that can be a general issue with passing functions around in any functional programming language. I saw your comment (maybe an Eastwood issue?) about ripping out and replacing the callback stuff. If that helps you out, I have no objections.

Since we basically have two usecases here. One is reporting progress as we go along, that would be eastwood.lint/eastwood, the other is summarizing the findings as data which would be eastwood.lint/lint

I guess one way to solve it would be to have some protocol which defined “warning” “exception” and “note” kind of functions and have two implementations of that which we could pass along.

Another way would be to include core.async as a lib and have the results pushed onto a channel which could then be read and the data could either be collected or printed.

Yet another way would be to have a global atom that holds the linter state, and install watchers on that atom that could print progress.

I don't have experience using core.async personally, but it seems like kind of a big dependency to take on for this purpose. A global atom is maybe ugly from the 'global variable' perspective, but a lot less machinery involved.

Eastwood code runs in the same JVM that the user's code is linted, and thus eval'd, so using core.async on projects that use core.async means adding a copy of core.async to the copieddeps directory.

yeah, I think core.async is out of the question as well.

I believe passing a thing that “implements” an IEastwoodReporter protocol would be my preferred way.

Which could be initialized with the values from opts, like :debug and :out

justin.smith@C02RW05WFVH6: ~/foo$ cat src/foo/core.clj 
(ns foo.core
  (:require [clojure.srting]
            [clojure.walk :as w]))

(def bar
  #{::w/foo})

(defn foo
  "I don't do a whole lot."
  [x]
  (println x "Hello, World!" bar))

justin.smith@C02RW05WFVH6: ~/foo$ lein eastwood
== Eastwood 0.3.1 Clojure 1.8.0 JVM 1.8.0_171
Directories scanned for source files:
  src test
== Linting foo.core ==
Linting failed:
ExceptionInfo foo/core.clj [line 6, col 12] Invalid keyword: ::w/foo.

minimal repro of the namespace keyword false-positive

the real error is srting not being string

the thing that made it hard to reproduce is that the problem isn't even in a related ns to the one the keyword aliases to

Does Clojure compile that file without error?

I would think it would throw an exception trying to eval the ns form?

no, it gives the expected error that it can't find clojure.srting

the problem is that eastwood is only erroring on ::w/foo, not the ns form

So Eastwood is continuing on past that exception, rather than stopping as i would hope it would.

right, and also reporting the wrong error

e.g. I would expect Eastwood should raise the same exception when eval'ing the ns form that Clojure would, and stop.

:thumbsup:

Out of curiosity, can you try an older version of Eastwood on that same file, e.g. maybe 0.2.6 or 0.2.7, to see if they raise an exception and stop? There may be changes in the exception handling since then, is my guess.

I'll try some time today, sure

I can try it later, too, so no rush. You want to file an issue, or you'd prefer I do?

btw I'm running into these issues because eastwood is saving me a lot of time and labor doing complex refactors and validating the results, so thanks :D

I'll plan to file an issue and compare to what earlier eastwood versions do later today

Cool. It is always nice to see minimal consistent reproducible problems, rather than "my 200 file project that I can't show you the source code of gives this error".

and I know those can take effort to create.

(both the 200 file project, and the minimal reproductions of the problems :-)

haha, right

reproduced the error on versions back to 0.2.4 so far

@andy.fingerhut I'll make an issue for this, the most recent eastwood version that catches this correctly is 0.1.3 (!)

OK, so said finger I was planning to surreptitiously point at @slipset now correctly belongs at me 🙂

oh me, of little trust.

https://github.com/jonase/eastwood/issues/294