marginalia doesn't parse namespaced keywords about marginalia HOT 14 CLOSED

I think the problem is that the clojure.lang.LispReader used in parse* does not know about any namespace aliases previously read.

See below. My guess is the namespace has to be require'd/use'd for namespace aliases to be recognized by the runtime environment.

  (let [source-string "(require '[marginalia.parser :as m])::m/asdf"
        make-reader #(java.io.BufferedReader.
                      (java.io.StringReader. (str source-string "\n")))
        lines (vec (line-seq (make-reader)))
        reader (clojure.lang.LineNumberingPushbackReader. (make-reader))
        old-cmt-rdr (aget (get-field clojure.lang.LispReader :macros nil) (int \;))]
    (doall 
      (take 2
            (repeatedly
              (fn []
                (. clojure.lang.LispReader
                   (read reader false nil false)))))))
#<CompilerException java.lang.RuntimeException: java.lang.RuntimeException: clojure.lang.LispReader$ReaderException: java.lang.Exception: Invalid token: ::m/asdf (REPL:113)>

from marginalia.

I pushed a potential fix for this. Try it out at http://clojars.org/marginalia (version 0.6.1)

from marginalia.

Keywords in Clojure can be prefixed with any valid symbol. Doing ::foo just so happens to use the ns as that prefix. Marg was always able to handle the keyword, but it was not dispatching properly during parse.

from marginalia.

AFAICT I still have problems.

This is the culprit for me: https://github.com/swannodette/match/blob/master/test/match/test/core.clj#L314

from marginalia.

WHUT!? ::m/foo is not a legal keyword. I know it works, but still -- that's a bug methinks.

from marginalia.

I've changed to fully qualified keywords for core.match, it works without a hitch.

from marginalia.

Great news. I'll talk to Core about this particular issue. This is edge-case city. :-)

from marginalia.

population: 2

from marginalia.

::m/foo is valid shorthand for ::match.core/foo, provided (require '[match.core :as m]) has been specified:

(require '[clojure.set :as set])
::set/foo
; => :clojure.set/foo

The way this happens inside clojure.lang.LispReader seems quite deliberate -- not likely a bug.

I have a possible fix at the issue-54-namespaced-keywords branch of my fork. The idea is to keep track of the current read-time namespace as forms are being read and evaluate ns, in-ns, require, use and alias forms immediately when they are encountered, so that any the read-time environment is set up appropriately before the next form is processed by the reader.

It might have been simpler to require any namespaces defined by ns or in-ns rather than evaluate the namespace-manipulating forms. I decided against this approach partly because of some vague hunches I had, and partly because writing the test was easier that way (no need to create a namespace and install an alias in it before running the actual test). I can provide an alternative patch based on this approach if desired.

from marginalia.

@fogus: Oh, I somehow missed the comment about you planning to talk to others at Core about this. I certainly hope this will not be deemed an undesirable feature, I quite like the shorthand. :-)

from marginalia.

That syntax was definitely new to me, and I was unable to find a reference to it. However, I will roll in your patch as it's generally useful. I did forget to bring it up last week, but I will do so this week. :-) Thank you for the patch. I appreciate your work.

from marginalia.

Great, thanks! This is a fantastic project and I'm really happy to be able to contribute.

from marginalia.

Turns out that attempt at a solution was a bit of a blunder -- if the ns forms and the like are evaluated, but nothing else is, any :use clauses / use calls referring to namespaces which have not already been require'd by the time Marginalia's parse sees them are guaranteed to refer to non-existing Vars, which leads straight to a crash.

Thus, I committed a reversal of the last patch on my topic branch and came up with a new approach which simply installs a custom reader for keywords (and then uninstalls it after parsing, of course). This is actually more robust, since ::foo/bar might be valid in some codebase by virtue of a call to use being made inside a top-level function call, say -- a situation which wouldn't have been handled well by the previous approach even if all the relevant namespaces were loaded.

A more detailed discussion is included in the commit message; and here is a link to the topic branch.

Sorry for the earlier breakage... Here's hoping no similar discoveries w.r.t. the new approach are forthcoming!

from marginalia.

I went ahead and pushed the fix described above; the previous version was known broken, so this can't be any worse, and who knows, it might actually work! ;-)

from marginalia.