Skip to content

Commit

Permalink
[#133] Don't throw if no ns form in the .clj file
Browse files Browse the repository at this point in the history
  • Loading branch information
benedekfazekas committed Mar 8, 2016
1 parent 8cc9dcf commit c34303d
Show file tree
Hide file tree
Showing 4 changed files with 13 additions and 11 deletions.
10 changes: 4 additions & 6 deletions src/refactor_nrepl/core.clj
Original file line number Diff line number Diff line change
Expand Up @@ -237,17 +237,15 @@
Dialect is either :clj or :cljs."
([path]
(with-open [file-reader (FileReader. path)]
(if-let [ns-form (parse/read-ns-decl (readers/indexing-push-back-reader
(when-let [ns-form (parse/read-ns-decl (readers/indexing-push-back-reader

This comment has been minimized.

Copy link
@expez

expez Mar 8, 2016

Member

Have you verified (by inspection) that all callers of these functions are able to handle a return value of nil?

This comment has been minimized.

Copy link
@benedekfazekas

benedekfazekas Mar 8, 2016

Author Member

nope. I trusted our test coverage and added that empty .clj file to trigger any possible errors.

This comment has been minimized.

Copy link
@expez

expez Mar 8, 2016

Member

I think that's a bit optimistic. IME it's a lot easier to read over the code to check that nil is handled correctly than to track down any resulting bugs weeks later.

(PushbackReader. file-reader)))]
(with-meta ns-form (extract-ns-meta (slurp path)))
(throw (IllegalStateException. (str "No ns form at " path))))))
(with-meta ns-form (extract-ns-meta (slurp path))))))
([dialect path]
(with-open [file-reader (FileReader. path)]
(if-let [ns-form (parse/read-ns-decl (readers/indexing-push-back-reader
(when-let [ns-form (parse/read-ns-decl (readers/indexing-push-back-reader
(PushbackReader. file-reader))
{:read-cond :allow :features #{dialect}})]
(with-meta ns-form (extract-ns-meta (slurp path)))
(throw (IllegalStateException. (str "No ns form at " path)))))))
(with-meta ns-form (extract-ns-meta (slurp path)))))))

(defn path->namespace
"Read the ns form found at PATH and return the namespace object for
Expand Down
7 changes: 5 additions & 2 deletions src/refactor_nrepl/ns/libspecs.clj
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@
(mapcat identity)
(apply hash-map)))

(defn- cache-contains? [f lang]
(-> @cache (get (.getAbsolutePath f)) (contains? lang)))

(defn- get-cached-libspec [f lang]
(when-let [[ts v] (get-in @cache [(.getAbsolutePath f) lang])]
(when (= ts (.lastModified f))
Expand All @@ -36,8 +39,8 @@
libspecs))

(defn- get-libspec-from-file-with-caching [lang f]
(if-let [v (get-cached-libspec f lang)]
v
(if (cache-contains? f lang)

This comment has been minimized.

Copy link
@expez

expez Mar 8, 2016

Member

Why was this change necessary?

The new version will return nil instead of a libspec if the modification time doesn't match. I don't see why that's a good idea.

This comment has been minimized.

Copy link
@benedekfazekas

benedekfazekas Mar 8, 2016

Author Member

yup you might be right here. will fix.

(get-cached-libspec f lang)
(put-cached-libspec f lang)))

(defn namespace-aliases
Expand Down
5 changes: 2 additions & 3 deletions test/refactor_nrepl/ns/clean_ns_test.clj
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,8 @@
(clean-ns ns-with-exclude))))

(deftest throws-on-malformed-ns

This comment has been minimized.

Copy link
@expez

expez Mar 8, 2016

Member

gotta rename this test now

This comment has been minimized.

Copy link
@benedekfazekas

benedekfazekas Mar 8, 2016

Author Member

yup, true

(is (thrown? IllegalStateException
(core/read-ns-form (.getAbsolutePath
(File. "test/resources/clojars-artifacts.edn"))))))
(is (not (core/read-ns-form (.getAbsolutePath
(File. "test/resources/clojars-artifacts.edn"))))))

(deftest preserves-other-elements
(let [actual (clean-ns ns1)
Expand Down
2 changes: 2 additions & 0 deletions test/resources/testproject/src/com/example/empty_file.clj
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
;; empty file
;; to test that empty, no ns form clj files don't break refactor-nrepl indexers

0 comments on commit c34303d

Please sign in to comment.