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

Support reader conditionals #194

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@arrdem
Copy link
Collaborator

arrdem commented May 9, 2017

This makes reader conditionals at least sort of work. The problem here is that because reader conditionals are correctly supported, kibit with --replace will now rewrite reader conditionals with their expanded form(s) which is quite likely undesired.

Not sure what better behavior looks like here. It may be that we can trap the reader conditional exception and just skip the top level form on it, although this renders kibit pretty useless on namespaces with lots of conditional forms.

Fixes #191 sorta kinda maybe not the way we want.

@arrdem arrdem force-pushed the arrdem:feature/reader-conditionals branch from 98cb4fe to c7f0647 May 9, 2017

@arrdem

This comment has been minimized.

Copy link
Collaborator Author

arrdem commented May 9, 2017

An alternate patch is

diff --git a/kibit/src/kibit/check.clj b/kibit/src/kibit/check.clj
index 0bb373e..4870d94 100644
--- a/kibit/src/kibit/check.clj
+++ b/kibit/src/kibit/check.clj
@@ -55,22 +55,28 @@ into the namespace."
   ns)
 
 (def eof (Object.))
+(def reader-cond (Object.))
 
 (defn read-file
   "Generate a lazy sequence of top level forms from a
    LineNumberingPushbackReader"
   [^LineNumberingPushbackReader r init-ns]
-  (let [do-read (fn do-read [ns]
-                  (lazy-seq
-                    (let [form (binding [*ns* ns]
-                                 (read r false eof))
-                          [ns? new-ns k] (when (sequential? form) form)
-                          ns (if (and (symbol? new-ns)
-                                      (or (= ns? 'ns) (= ns? 'in-ns)))
-                               (careful-refer (create-ns new-ns))
-                               ns)]
-                      (when-not (= form eof)
-                        (cons form (do-read ns))))))]
+  (let [do-read
+        (fn do-read [ns]
+          (lazy-seq
+           (let [form           (binding [*ns* ns]
+                                  (try (read r false eof)
+                                       (catch Exception e
+                                         reader-cond)))
+                 [ns? new-ns k] (when (sequential? form) form)
+                 ns             (if (and (symbol? new-ns)
+                                         (or (= ns? 'ns) (= ns? 'in-ns)))
+                                  (careful-refer (create-ns new-ns))
+                                  ns)]
+             (when-not (= form eof)
+               (if (= form reader-cond)
+                 (do-read ns)
+                 (cons form (do-read ns)))))))]
     (do-read (careful-refer (create-ns init-ns)))))
 
 ;; ### Analyzing the pieces

which just masks reader exceptions and returns a sentinel object so that we skip the form containing the reader conditional and keep going.

@jmromrell

This comment has been minimized.

Copy link

jmromrell commented May 9, 2017

Just saw this, I actually had just come to the same result as your first fix.

I'd personally consider desirable behavior to fully support reader conditionals, as per your first fix, and fix the --remove functionality separately, disabling it again for now if necessary.

I feel like being able to properly provide suggestions for all files (including those with reader conditionals) is more essential towards fulfilling the purpose of kibit than automatic replacement, which users have lived without thus far and the several bugs I reported.

@arrdem

This comment has been minimized.

Copy link
Collaborator Author

arrdem commented May 9, 2017

Yet another option here is to use the "preserve reader conditionals" mode for read, but who knows what that'll do to the kibit pattern matcher. At least it'll sorta work I guess.....

@arrdem

This comment has been minimized.

Copy link
Collaborator Author

arrdem commented May 9, 2017

So I don't think that the "right" thing to do here is to teach kibit about reader conditionals at all. At best, I think that reading in a conditional-preserving mode which happens to probably prevent most kibit rewrites is the most sane thing to do next to just skipping forms containing reader conditionals entirely.

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