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

info: don't use a fallback for ns-qualified symbols #134

Merged
merged 6 commits into from
Oct 2, 2021
Merged

info: don't use a fallback for ns-qualified symbols #134

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
3801e8f
`info`: don't use a fallback for ns-qualified symbols
vemv Oct 2, 2021
2db7cb0
Add `make install` command
vemv Oct 2, 2021
61234d9
Unflake a test
vemv Oct 2, 2021
3565c39
Satisfy clj-kondo
vemv Oct 2, 2021
42b74ab
Further increase test coverage
vemv Oct 2, 2021
a0c45e8
0.7.3
vemv Oct 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## master (unreleased)

### Changes

* [#133](https://github.com/clojure-emacs/orchard/issues/133): `info:` don't fall back to `clojure.core` for fully-qualified symbols.

## 0.7.2 (2021-09-30)

### Changes
Expand Down
21 changes: 12 additions & 9 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,35 +8,38 @@ resources/clojuredocs/export.edn:
curl -o $@ https://github.com/clojure-emacs/clojuredocs-export-edn/raw/master/exports/export.compact.edn

test:
lein with-profile +$(VERSION),$(TEST_PROFILES) test
lein with-profile -user,-dev,+$(VERSION),$(TEST_PROFILES) test

test-watch: test-resources/clojuredocs/export.edn
lein with-profile +$(VERSION),$(TEST_PROFILES) test-refresh

eastwood:
lein with-profile +$(VERSION),+eastwood,$(TEST_PROFILES) eastwood
lein with-profile -user,-dev,+$(VERSION),+eastwood,$(TEST_PROFILES) eastwood

cljfmt:
lein with-profile +$(VERSION),+cljfmt cljfmt check
lein with-profile -user,-dev,+$(VERSION),+cljfmt cljfmt check

kondo:
lein with-profile -dev,+clj-kondo run -m clj-kondo.main --lint src test src-jdk8 src-newer-jdks
lein with-profile -user,-dev,+clj-kondo run -m clj-kondo.main --lint src test src-jdk8 src-newer-jdks

# When releasing, the BUMP variable controls which field in the
# version string will be incremented in the *next* snapshot
# version. Typically this is either "major", "minor", or "patch".

BUMP ?= patch

release:
lein with-profile +$(VERSION) release $(BUMP)
release: clean
lein with-profile -user,-dev,+$(VERSION) release $(BUMP)

# Deploying requires the caller to set environment variables as
# specified in project.clj to provide a login and password to the
# artifact repository.

deploy:
lein with-profile +$(VERSION) deploy clojars
deploy: clean
lein with-profile -user,-dev,+$(VERSION) deploy clojars

install: clean
lein with-profile -user,-dev,+$(VERSION) install

clean:
lein clean
lein with-profile -user,-dev clean
53 changes: 25 additions & 28 deletions src/orchard/info.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@
[orchard.cljs.meta :as cljs-meta]
[orchard.java :as java]
[orchard.java.classpath :as cp]
[orchard.java.resource :as resource]
[orchard.meta :as m]
[orchard.misc :as misc]
[orchard.java.resource :as resource]))
[orchard.misc :as misc]))

(defn qualify-sym
"Qualify a symbol, if any in `sym`, with `ns`.
Expand Down Expand Up @@ -65,32 +65,29 @@
{:added "0.5"}
[{:keys [dialect ns sym computed-ns unqualified-sym] :as opts}]
{:pre [(= dialect :clj)]}
(let [ns (or ns computed-ns)]
(if-not (and ns (find-ns ns))
;; Lookups in files whose namespaces don't exist yet should still be able
;; to resolve built-ins and fully-qualified syms
(recur (assoc opts :ns 'clojure.core))
(or
;; it's a special (special-symbol?)
(m/special-sym-meta sym)
;; it's a var
(some-> ns (m/resolve-var sym) (m/var-meta))
;; it's a Java constructor/static member symbol
(some-> ns (java/resolve-symbol sym))
;; it's an unqualified sym maybe referred
(some-> ns (m/resolve-var unqualified-sym) (m/var-meta))
Comment on lines -80 to -81
Copy link
Member Author

@vemv vemv Oct 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Removed these two lines, I don't think they make sense. If one is explicitly querying :sym foo/+ (i.e. fully-qualified), then trying to resolve + without foo is not accurate.

The comment says maybe referred. But (some-> ns (m/resolve-var sym) (m/var-meta))a few lines above already handles refers, that's part of what resolve-var/ns-resolve do

;; it's a Java class/record type symbol
(some-> ns (java/resolve-type unqualified-sym))
;; it's an alias for another ns
(some-> ns (m/resolve-aliases) (get sym) (m/ns-meta))
;; We use :unqualified-sym *exclusively* here because because our :ns is
;; too ambiguous.
;;
;; Observe the incorrect behavior (should return nil, there is a test):
;;
;; (info '{:ns clojure.core :sym non-existing}) ;;=> {:author "Rich Hickey" :ns clojure.core ...}
;;
(some-> (find-ns unqualified-sym) (m/ns-meta))))))
(let [ns (or ns computed-ns)
ns (or (when (some-> ns find-ns)
ns)
'clojure.core)]
(or
;; it's a special (special-symbol?)
(m/special-sym-meta sym)
;; it's a var
(some-> ns (m/resolve-var sym) (m/var-meta))
;; it's a Java constructor/static member symbol
(some-> ns (java/resolve-symbol sym))
;; it's a Java class/record type symbol
(some-> ns (java/resolve-type unqualified-sym))
;; it's an alias for another ns
(some-> ns (m/resolve-aliases) (get sym) (m/ns-meta))
;; We use :unqualified-sym *exclusively* here because because our :ns is
;; too ambiguous.
;;
;; Observe the incorrect behavior (should return nil, there is a test):
;;
;; (info '{:ns clojure.core :sym non-existing}) ;;=> {:author "Rich Hickey" :ns clojure.core ...}
;;
(some-> (find-ns unqualified-sym) (m/ns-meta)))))

(defn cljs-meta
{:added "0.5"}
Expand Down
63 changes: 43 additions & 20 deletions test/orchard/info_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
(ns orchard.info-test
(:require
[clojure.test :as test :refer [deftest is testing use-fixtures]]
[clojure.string :as str]
[clojure.java.io :refer [resource]]
[clojure.string :as str :refer [replace-first]]
[clojure.test :refer [are deftest is testing use-fixtures]]
[orchard.cljs.test-env :as test-env]
[orchard.info :as info]
[orchard.java :as java]
[orchard.misc :as misc]
[clojure.java.io :refer [resource]]
[orchard.cljs.test-env :as test-env]
[orchard.test-ns]))

@java/cache-initializer ;; make tests more deterministic
Expand Down Expand Up @@ -352,7 +352,8 @@

(deftest info-macros-referred-var-test
(testing "Macro - referred"
(let [params '[{:sym orchard.test-ns/my-add}
(let [params '[{:sym orchard.test-ns/my-add},

{:ns orchard.test-ns
:sym my-add}]
expected '{:name my-add
Expand All @@ -368,7 +369,7 @@
(map #(select-keys % [:ns :name :arglists :macro :file]))))))

(testing "- :clj"
(is (= (take 2 (repeat expected))
(is (= [{}, expected]
Copy link
Member Author

Choose a reason for hiding this comment

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

Changed this expectation, orchard.test-ns/my-add does not exist so the right result for it is {}

(->> params
(map #(info/info* %))
(map #(select-keys % [:ns :name :arglists :macro :file])))))))))
Expand Down Expand Up @@ -467,21 +468,43 @@
:returns int}))
(is (re-find #"Returns the greater of two" (:doc i))))))

(def some-var nil)

(deftest info-undefined-namespace-test
(testing "Fully qualified sym can still be resolved"
(is (= '{:added "1.2"
:ns clojure.string
:name upper-case
:file "clojure/string.clj"}
(select-keys (info/info* {:ns 'gibberish :sym 'clojure.string/upper-case})
[:added :ns :name :file]))))
(testing "clojure.core syms can still be resolved"
(is (= '{:added "1.0"
:ns clojure.core
:name merge
:file "clojure/core.clj"}
(select-keys (info/info* {:ns 'gibberish :sym 'merge})
[:added :ns :name :file])))))
(let [current-ns (-> ::_ namespace symbol)]
(are [input expected] (= expected
Copy link
Member

Choose a reason for hiding this comment

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

I don't like are as I think it reduces the readability of tests. For me it's better to have more testing blocks, as they make it easier to follow what a group of assertions tests exactly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps we can agree to disagree? I didn't use an are of the crazy flavor here. It's a simple mapping of inputs->outputs.

Normally to reach this level of exhaustiveness with manually coded tests, one would have to take 1 to 2 'screenfuls' worth of code. That's generally not too readable - it's so wordy that it's hard to compare the test cases between them.

Normally people give up before even attempting this level of exhaustiveness, so are can invite us to do the right thing (focus on the data, compare the tests cases between them, cheaply add more cases if missing)

Copy link
Member

Choose a reason for hiding this comment

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

I can live with this, I simply don't like it. Too many years of writing RSpecs I guess. :-) There's also the practical problem that CIDER's test runner doesn't work well with are, but that's not a big deal.

(select-keys (info/info* input)
[:added :ns :name :file]))
{:ns current-ns :sym 'some-var} '{:ns orchard.info-test,
:name some-var,
:file "orchard/info_test.clj"}
{:ns current-ns :sym 'replace-first} '{:added "1.2",
:ns clojure.string,
:name replace-first,
:file "clojure/string.clj"}
{:ns current-ns :sym 'merge} '{:added "1.0"
:ns clojure.core
:name merge
:file "clojure/core.clj"}
{:ns current-ns :sym 'non.existing.ns/merge} {}
{:ns current-ns :sym 'clojure.string/upper-case} '{:added "1.2"
:ns clojure.string
:name upper-case
:file "clojure/string.clj"}
{:ns current-ns :sym 'non.existing.ns/upper-case} {}

{:ns 'gibberish :sym 'some-var} {}
{:ns 'gibberish :sym 'replace-first} {}
{:ns 'gibberish :sym 'merge} '{:added "1.0"
:ns clojure.core
:name merge
:file "clojure/core.clj"}
{:ns 'gibberish :sym 'non.existing.ns/merge} {}
{:ns 'gibberish :sym 'clojure.string/upper-case} '{:added "1.2"
:ns clojure.string
:name upper-case
:file "clojure/string.clj"}
{:ns 'gibberish :sym 'non.existing.ns/upper-case} {})))

(deftest javadoc-info-unit-test
(testing "Get an HTTP URL for a Sun/Oracle Javadoc"
Expand Down
4 changes: 3 additions & 1 deletion test/orchard/java/resource_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

(deftest project-resources-test
(testing "get the correct resources for the orchard project"
(let [resources (resource/project-resources)]
(let [resources (->> (resource/project-resources)
Copy link
Member

Choose a reason for hiding this comment

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

What was wrong with this test before? There were more resources returned or something?

Copy link
Member Author

@vemv vemv Oct 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

There were more resources returned or something?

Yes especially in local envs where there can be more resources for arbitrary reasons.

Copy link
Member

Choose a reason for hiding this comment

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

Got it.

(filter (fn [{:keys [relpath]}]
(= relpath "clojuredocs/test_export.edn"))))]
(is (= "clojuredocs/test_export.edn" (-> resources first :relpath)))
(is (= java.net.URL (-> resources first :url class))))))