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

info op fails for cljs macro in aliased namespace #106

Closed
jobo3208 opened this issue Feb 12, 2021 · 5 comments
Closed

info op fails for cljs macro in aliased namespace #106

jobo3208 opened this issue Feb 12, 2021 · 5 comments

Comments

@jobo3208
Copy link
Contributor

Expected behavior

(ns my-ns.core
  (:require
    [cljs.spec.alpha :as s]))

Given this ns, when I do an info op on s/keys (a macro), it should return the macro metadata.

Actual behavior

Instead, it returns nil. The info op works fine for functions like s/explain. It also works fine if I :refer the macro explicitly.

Steps to reproduce the problem

I think the above illustrates the steps needed to reproduce.

I believe the issue is here:

(let [ns (or context-ns (macro-namespace env sym context-ns))

In my example, macro-namespace is never called, so the code instead looks for the macro in my-ns.core, where it doesn't exist. I took the naive approach of simply swapping the two arguments to or and it seems to work for me. I can submit a pull request, but wanted to run it by you first to see if this solution even makes sense (I'm still fairly new to Clojure, and especially new to this codebase).

Environment & Version information

Clojure version

1.10.0

Java version

openjdk 11.0.10

Operating system

Debian unstable

@bbatsov
Copy link
Member

bbatsov commented Feb 12, 2021

The suggested fix makes sense to me. I don't really remember why the code is written as it is, because I think we always submit the context-ns, which is what you've encountered. Perhaps @arichiardi can provide more input here.

At any rate - PR welcome!

@arichiardi
Copy link
Contributor

Fortunately we should have good tests there so we might as well try. The tricky part of that piece of code is exactly ordering...hopefully this will not be one of those case where it breaks.

jobo3208 added a commit to jobo3208/orchard that referenced this issue Feb 12, 2021
@bbatsov
Copy link
Member

bbatsov commented Feb 12, 2021

I guess we didn't have a test for that particular problem, though. :-)

@arichiardi
Copy link
Contributor

Maybe we should add one?

@arichiardi
Copy link
Contributor

I will try to see if I can get back into this, this year was full for me 👶

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants