Fix importall warning (for real this time)

[ahwillia]

I'm not sure if this issue (#1) was actually resolved. The warning was persisting for me, I think the issue was that mod needed to be interpolated and turned into a symbol.

This PR fixes the problem for me.

julia> module A end
julia> module B
           using Reexport
           importall A
           @reexport using A
       end
julia> using B
# nothing but crickets here

[ahwillia]

Ugh. Of course I was over-confident and forgot to test before opening the PR, hence the second commit. I can squash them before merging if you think this is a reasonable fix.

[simonster]

When I run the code above, I don't see any warnings on either 0.4 or 0.5. Can you provide more details?

[ahwillia]

Weird. I uninstalled Reexport and ran the following test just to make sure... I get the exact same on v0.5 (4 days old master).

               _
   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: http://docs.julialang.org
   _ _   _| |_  __ _   |  Type "?help" for help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.4.6 (2016-06-19 17:16 UTC)
 _/ |\__'_|_|_|\__'_|  |  Official http://julialang.org/ release
|__/                   |  x86_64-apple-darwin13.4.0

julia> Pkg.add("Reexport")
INFO: Installing Reexport v0.0.3
INFO: Package database updated

julia> module A end
A

julia> module B
    using Reexport
    importall A
    @reexport using A
end
INFO: Recompiling stale cache file /Users/alex/.julia/lib/v0.4/Reexport.ji for module Reexport.
B

julia> using B
WARNING: using B.A in module Main conflicts with an existing identifier.

julia> Pkg.checkout("Reexport")
INFO: Checking out Reexport master...
INFO: Pulling Reexport latest master...
INFO: No packages to install, update or remove

julia> module C end
C

julia> module D
           using Reexport
           importall C
           @reexport using C
       end
D

julia> using D
WARNING: using D.C in module Main conflicts with an existing identifier.

What do you get when you run the following?

## For the current release ##
using Reexport
macroexpand(:(@reexport using A))

Before the PR I get:

:($(Expr(:toplevel, :(using A), :(eval(Expr(:export,setdiff(names(A),[mod])...))))))

Afterwards I get something that (I think) makes more sense:

:($(Expr(:toplevel, :(using A), :(eval(Expr(:export,setdiff(names(A),[Symbol(string(A))])...))))))

[Evizero]

I also have the importall issue currently. I find this proposed solution a bit unintuitive.

wouldn't @reexport importall ModuleA be a more intuitive syntax? it seems that all that needs to change is that the if statements that check for == :using include an OR for == :importall.

I shall investigate

[Evizero]

Oh, now I see what your PR is solving. The mod variable isn't replaced with the symbol for the Package, and as such the module name is also reexported which causes the warning. i.e. I can reproduce @ahwillia 's issue

[Evizero]

regardless, @ahwillia if you add this diff, then @reexport importall ModuleAshould also work

diff --git a/src/Reexport.jl b/src/Reexport.jl
index b14d0ca..543af53 100644
--- a/src/Reexport.jl
+++ b/src/Reexport.jl
@@ -5,6 +5,7 @@ module Reexport
 macro reexport(ex)
     isa(ex, Expr) && (ex.head == :module ||
                       ex.head == :using ||
+                      ex.head == :importall ||
                       (ex.head == :toplevel &&
                        all(e->isa(e, Expr) && e.head == :using, ex.args))) ||
         error("@reexport: syntax error")
@@ -12,7 +13,7 @@ macro reexport(ex)
     if ex.head == :module
         modules = Any[ex.args[2]]
         ex = Expr(:toplevel, ex, Expr(:using, :., ex.args[2]))
-    elseif ex.head == :using
+    elseif ex.head == :using || ex.head == :importall
         modules = Any[ex.args[end]]
     else
         modules = Any[e.args[end] for e in ex.args]

[simonster]

Implemented @Evizero's solution and removed the setdiff that I agree wasn't doing anything.

It's still arguable that we shouldn't be reexporting the module itself, and only the symbols it exports. If we wanted to skip the module, then the correct symbol to be excluded is $(Meta.quot(mod)), not Symbol(string($mod)), since string($mod) will be A.B and not B for a nested module. But I'm kind of scared to make that change, since packages might be depending on the current behavior.

[simonster]

Also, can someone confirm that using @reexport importall X with the latest master fixes this? I still can't reproduce the issue on my system.

[Evizero]

Just tested it. The reexport part works for me now, thanks! The warning still exists. Not sure if that does harm or not

WARNING: using Losses.LearnBase in module Main conflicts with an existing identifier.

EDIT: don't think it does though, since all tests pass

[ahwillia]

I can confirm @Evizero's experience. I still get the warning. I see the potential problem if packages are depending on the current behavior, but to me it seems the following two should reexport the same thing (i.e. without the module):

@reexport using A

importall A
@reexport using A

And if you want, this could export a binding to the module itself:

@reexport importall A

[joshday]

importall is now deprecated (JuliaLang/julia#22789), so this could probably be closed.