info: don't use a fallback for ns-qualified symbols
#134
Conversation
Also includes our usual refinements (`-user` etc).
| ;; it's an unqualified sym maybe referred | ||
| (some-> ns (m/resolve-var unqualified-sym) (m/var-meta)) |
There was a problem hiding this comment.
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
test/orchard/info_test.clj
Outdated
| @@ -368,7 +369,7 @@ | |||
| (map #(select-keys % [:ns :name :arglists :macro :file])))))) | |||
|
|
|||
| (testing "- :clj" | |||
| (is (= (take 2 (repeat expected)) | |||
| (is (= [{}, expected] | |||
There was a problem hiding this comment.
Changed this expectation, orchard.test-ns/my-add does not exist so the right result for it is {}
vemv
changed the title
info: don't fall back to clojure.core for fully-qualified symbols
vemv
changed the title
info: don't use a fallback for ns-qualified symbols
| (select-keys (info/info* {:ns 'gibberish :sym 'merge}) | ||
| [:added :ns :name :file]))))) | ||
| (let [current-ns (-> ::_ namespace symbol)] | ||
| (are [input expected] (= expected |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
| @@ -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) | |||
There was a problem hiding this comment.
What was wrong with this test before? There were more resources returned or something?
There was a problem hiding this comment.
There were more resources returned or something?
Yes especially in local envs where there can be more resources for arbitrary reasons.
|
0.7.3 released with these changes and consumed in cider-nrepl clojure-emacs/cider-nrepl#715 |
|
Thanks for picking this up so quickly! |
|
🍻 Turned out that 7d7f752 wasn't wrong per se but it helped surface various edge cases (covered with tests here and downstream) |
info: don't use a fallback for ns-qualified symbolsmake installcommand-useretc).