Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[WIP] Fix bug with no :file for info on namespaces w/o defs #76

Merged
merged 6 commits into from Nov 5, 2019
Merged

[WIP] Fix bug with no :file for info on namespaces w/o defs #76

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
754ae06
Add test exposing issue #75
PEZ Nov 1, 2019
8eadc1a
Add basic ns-meta-test
PEZ Nov 1, 2019
66b8f5c
Use alt method for file paths when no ns-publics
PEZ Nov 2, 2019
ff80f85
Refactor out `ns-file`
PEZ Nov 2, 2019
fb60d19
Make `canonical-source` handle `.cljs` files
PEZ Nov 2, 2019
1a65423
Add canonical-source fallback for cljs dialect
PEZ Nov 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Expand Up @@ -9,3 +9,5 @@ pom.xml
pom.xml.asc
*.jar
*.class
.cljs_nashorn_repl/
nashorn_code_cache/
14 changes: 9 additions & 5 deletions src/orchard/meta.clj
View file
Open in desktop
Expand Up @@ -268,11 +268,15 @@
(meta ns)
{:ns (ns-name ns)
:name (ns-name ns)
:file (-> (ns-publics ns)
first
second
var-meta
:file)
:file (or
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you can extract this as a function - ns-file.

(-> (ns-publics ns)
first
second
var-meta
:file)
(->
(ns/canonical-source ns)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guessing when you try this manually it actually works (instead of returning nil), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, (ns-meta 'orchard.test-no-defs) returns the right thing. And the :clj ”branch” of the test also passes. But the :cljs part of the test does not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems canonical-source doesn't work with cljs files:

(defn canonical-source
  "Returns the URL of the source file for the namespace object or symbol,
  according to the canonical naming convention, if present on the classpath."
  [ns]
  (let [path (-> (str ns)
                 (str/replace "-" "_")
                 (str/replace "." "/"))]
    (or (io/resource (str path ".clj"))
        (io/resource (str path ".cljc")))))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! This would have made me fail fix BetterThanTomorrow/calva#427 even when we find out why the cljs tests fail. 😄

.getPath))
:line 1})))

;;; ## Manipulation
Expand Down
2 changes: 2 additions & 0 deletions test-resources/orchard/test_no_defs.cljc
View file
Open in desktop
@@ -0,0 +1,2 @@
(ns ^{:doc "Namespace w/o any `def`s, issue #75"}
orchard.test-no-defs)
3 changes: 2 additions & 1 deletion test-resources/orchard/test_ns.cljc
View file
Open in desktop
@@ -1,7 +1,8 @@
(ns ^{:doc "A test namespace"} orchard.test-ns
(:refer-clojure :exclude [unchecked-byte while])
(:require [clojure.string :refer [replace]]
[orchard.test-ns-dep :as test-dep :refer [foo-in-dep]])
[orchard.test-ns-dep :as test-dep :refer [foo-in-dep]]
[orchard.test-no-defs :as no-defs])
#?(:cljs (:require-macros [orchard.test-macros :as test-macros :refer [my-add]])
:clj (:require [orchard.test-macros :as test-macros :refer [my-add]]))
#?(:cljs (:import [goog.ui IdGenerator])))
Expand Down
1 change: 1 addition & 0 deletions test/orchard/cljs/env_test.cljc
View file
Open in desktop
Expand Up @@ -10,6 +10,7 @@
(is (empty? (set/difference (set (keys (a/all-ns env)))
'#{orchard.test-ns
orchard.test-ns-dep
orchard.test-no-defs
orchard.test-macros
cljs.core
cljs.user
Expand Down
15 changes: 14 additions & 1 deletion test/orchard/info_test.clj
View file
Open in desktop
Expand Up @@ -179,7 +179,7 @@
(testing "Resolution from current namespace - issue #28 from cljs-tooling"
(let [i (info/info* (merge *cljs-params* '{:ns orchard.test-ns :sym issue-28}))]
(is (= '{:arglists ([])
:line 14
:line 15
:column 1
:ns orchard.test-ns
:name issue-28}
Expand Down Expand Up @@ -385,6 +385,19 @@
(is (= expected (select-keys i [:ns :name :doc :forms :special-form :url])))
(is (nil? (:file i))))))))

(deftest file-resolution-no-defs-issue-75-test
(testing "File resolves, issue #75"
(let [params '{:ns orchard.test-ns
:sym orchard.test-no-defs}
cljs-merged-params (merge *cljs-params* params)
f "orchard/test_no_defs.cljc"]

(testing "- :cljs"
(is (.endsWith (:file (info/info* cljs-merged-params)) f)))

(testing "- :clj"
(is (.endsWith (:file (info/info* params)) f))))))

;;;;;;;;;;;;;;;;;;
;; Clojure Only ;;
;;;;;;;;;;;;;;;;;;
Expand Down
11 changes: 11 additions & 0 deletions test/orchard/meta_test.clj
View file
Open in desktop
Expand Up @@ -110,3 +110,14 @@
(docs/clean-cache!)
(testing "Including see-also is skipped"
(is (not (contains? (m/var-meta (resolve 'clojure.set/union)) :see-also))))))

(deftest ns-meta-test
(testing "Includes a non-nil :file"
(is (some-> 'orchard.test-ns-dep
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the need for some-> here? After all you expect this not to be nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the whole thread that should return something not nil, right? Maybe I do not follow the question...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad. Too much multitasking. :D

(find-ns)
(m/ns-meta)
:file))
(is (some-> 'orchard.test-no-defs ;; issue #75
(find-ns)
(m/ns-meta)
:file))))