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

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

Open
joodie opened this Issue Jun 26, 2014 · 6 comments

Comments

Projects
None yet
5 participants
@joodie
Copy link

joodie commented Jun 26, 2014

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

This comment has been minimized.

Copy link
Collaborator

guns commented Jun 26, 2014

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

This comment has been minimized.

Copy link
Collaborator

guns commented Jul 2, 2014

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

This comment has been minimized.

Copy link
Owner

technomancy commented Jul 3, 2014

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

This comment has been minimized.

Copy link
Collaborator

guns commented Jul 3, 2014

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

This comment has been minimized.

Copy link

jimrthy commented Aug 29, 2017

This seems more important with clojure 1.9 around the corner.

@jeaye

This comment has been minimized.

Copy link

jeaye commented Sep 25, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment