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.

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

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

Merged
merged 6 commits into from Nov 5, 2019

Conversation

@PEZ
Copy link
Contributor

PEZ commented Nov 2, 2019

Sending this PR in to get feedback.

The problem with :file entries being nil for namespaces where there are no defs is that orchard.meta/ns-meta relies on clojure.core/ns-publics to first pick out some of the def:ed vars in order to use var-meta on them.

In this PR I use a backup method for when ns-publics return an empty map.

However, in orchard.info-test/file-resolution-no-defs-issue-75-test I still get a nil back for the :cljs test. I have yet failed to figure that out. Any feedback on that is very welcome.

I'd also want some general feedback, as I am planning to try contribute to this library more, and I need to learn the best ways to do that. Cheers!

  • The commits are consistent with our contribution guidelines
  • You've added tests to cover your change(s)
  • All tests are passing
  • The new code is not generating reflection warnings
PEZ added 3 commits Nov 1, 2019
NB: Still get nil back when used with cljs-env, don't merge!
@bbatsov bbatsov requested a review from arichiardi Nov 2, 2019
second
var-meta
:file)
:file (or

This comment has been minimized.

Copy link
@bbatsov

bbatsov Nov 2, 2019

Member

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


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

This comment has been minimized.

Copy link
@bbatsov

bbatsov Nov 2, 2019

Member

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

This comment has been minimized.

Copy link
@PEZ

PEZ Nov 2, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@bbatsov

bbatsov Nov 2, 2019

Member

My bad. Too much multitasking. :D

@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented Nov 2, 2019

Your changes seem good me to me. Not sure what's causing the failure. Maybe @arichiardi would know.

var-meta
:file)
(->
(ns/canonical-source ns)

This comment has been minimized.

Copy link
@bbatsov

bbatsov Nov 2, 2019

Member

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

This comment has been minimized.

Copy link
@PEZ

PEZ Nov 2, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@bbatsov

bbatsov Nov 2, 2019

Member

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")))))

This comment has been minimized.

Copy link
@PEZ

PEZ Nov 2, 2019

Author Contributor

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

PEZ added 2 commits Nov 2, 2019
Then also rewrite `ns-meta-test` to test the whole result.
Copy link
Contributor

arichiardi left a comment

Code looks good to me too. I would have to debug in order to understand why it fails. Is this the only failing test? In any case I might have some free time next week and I can check it out.

@PEZ

This comment has been minimized.

Copy link
Contributor Author

PEZ commented Nov 2, 2019

Yes, the only failing test. I think I have found another clue now. ns-meta is not used for the :cljs dialect. 😄

I am now tracking down what is used there instead. Good to know that the rests looks good.

@arichiardi

This comment has been minimized.

Copy link
Contributor

arichiardi commented Nov 2, 2019

Yes the Cljs is more complicated because you need to get to the Compiler state. The wrappers though are are in the cljs specific namespaces (meta.cljs and analysis.cljs iirc).

Not a the keyboard now but I can help you with that at some point.

@arichiardi

This comment has been minimized.

Copy link
Contributor

arichiardi commented Nov 2, 2019

The clj-meta and cljs-meta are the entry points and we probably should check the fix for both branches of info

Move normalize-ns-meta to cljs-meta ns
(and remove old unused version lurking in there)
@PEZ

This comment has been minimized.

Copy link
Contributor Author

PEZ commented Nov 2, 2019

Thanks! Found it. Fixed it in a similar way as I did for the :clj dialect.

Also, the info ns had its own ”copy” of the normalize-ns-meta fn. It had evolved past the one in the cljs.meta namespace. It took me a while to realise the latter one was not being used. I choose to move the used version to the cljs.meta namespace and delete the one in there. So now there is only one. And it has the fix for this no-defs business.

Copy link
Contributor

arichiardi left a comment

If all tests pass this can be merged imho

@bbatsov bbatsov merged commit e474c7d into clojure-emacs:master Nov 5, 2019
9 checks passed
9 checks passed
ci/circleci: Code Linting Your tests passed on CircleCI!
Details
ci/circleci: Java 11, Clojure 1.10 Your tests passed on CircleCI!
Details
ci/circleci: Java 11, Clojure 1.8 Your tests passed on CircleCI!
Details
ci/circleci: Java 11, Clojure 1.9 Your tests passed on CircleCI!
Details
ci/circleci: Java 11, Clojure master Your tests passed on CircleCI!
Details
ci/circleci: Java 8, Clojure 1.10 Your tests passed on CircleCI!
Details
ci/circleci: Java 8, Clojure 1.8 Your tests passed on CircleCI!
Details
ci/circleci: Java 8, Clojure 1.9 Your tests passed on CircleCI!
Details
ci/circleci: Java 8, Clojure master Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.