"Invalid token" error when using abbreviated namespaced keywords

technomancy / slamhound

Slamhound rips your namespace form apart and reconstructs it.

Other

473 stars 38 forks source link

"Invalid token" error when using abbreviated namespaced keywords #79

Open joodie opened 10 years ago

joodie commented 10 years ago

Requiring a namespace using :as alias, then using the alias to refer to a keyword in that namespace throws an error in slamhound.

Given src/my_test.clj:

(ns my-test
  (:require [clojure.test :as test]))
(prn :clojure.test/foo) ;; works but no alias
(prn ::test/foo) ;; this line bugs slamhound

Results in:

> lein run -m slam.hound src/my_test.clj
Failed to reconstruct: #<File src/my_test.clj>
Invalid token: ::test/foo

I think slamhound should parse this as clojure does (AFAIKT this feature has been in clojure since 1.0, though documentation is scarce) and interprets ::alias/word as a requirement to keep the :as alias.

guns commented 10 years ago

Ah, that's interesting; I never considered the combination of auto-qualifying keywords and aliases. I'll take a look later today.

Thanks for the report!

guns commented 10 years ago

Hello, I finally had a chance to look at this today.

This Invalid token error occurs at read-time and therefore never reaches Slamhound's ns regrow loop.

It is possible to add a special-case just for this. It would require:

Breaking apart asplode/asplode into two separate processes: parse-ns and read-body. This changes the return type of asplode/asplode, and requires a bit of test refactoring
Adding a new regrow/check-for-read-failure fn that parses LispReader exceptions
Adding a new regrow/read-body fn, which is just a partial application of regrow/regrow + the new regrow/check-for-read-failure fn

At a high level then, the reconstruction algorithm becomes:

-> file
   asplode-ns
   read-body
   regrow-ns
   stitch-up

@technomancy: Do you think supporting ::alias/sym is worth the above changes? I am happy to do the work

technomancy commented 10 years ago

I'm not sure how this requires a new phase, since reading the body is already the last isolated step of asplode, but yeah, this seems reasonable.

guns commented 10 years ago

It doesn't require a new phase, but it avoids making asplode depend on regrow. I also think it's nicer if asplode returns a file's ns form without any interpretation.

Anyway, great! I will push up an implementation tomorrow to a feature branch for review.

jimrthy commented 7 years ago

This seems more important with clojure 1.9 around the corner.

jeaye commented 7 years ago

Yeah, we use this in nearly every ns we have. +1