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

Handle nested modules #52

Merged
merged 8 commits into from
Jul 11, 2018
Merged

Handle nested modules #52

merged 8 commits into from
Jul 11, 2018

Conversation

rofinn
Copy link
Collaborator

@rofinn rofinn commented Jul 10, 2018

Currently, using nested modules can result in an UndefVarError.

julia> using Mocking

julia> Mocking.enable(; force=true)
Warning: Using experimental code which modifies jl_options global struct

julia> module A

       module B

       abstract type AbstractFoo end

       struct Foo <: AbstractFoo
           x::String
       end

       end

       bar(f::B.AbstractFoo) = println("blah")

       end
A

julia> import A: bar

julia> patch = @patch bar(f::B.AbstractFoo) = println("blar")
Mocking.Patch(:(bar(f::A.B.AbstractFoo)), ##665, Set(Any[:(A.B)]))

julia> Mocking.apply(patch) do
           bar(B.Foo("Blar"))
       end
ERROR: UndefVarError: A not defined
Stacktrace:
 [1] Mocking.PatchEnv(::Mocking.Patch, ::Bool) at /Users/rory/.playground/share/mocking/packages/v0.6/Mocking/src/Mocking.jl:156
 [2] apply(::Function, ::Mocking.Patch) at /Users/rory/.playground/share/mocking/packages/v0.6/Mocking/src/Mocking.jl:184

This is because the patch is only importing the top level module while the method is using a fully specified type signature:

ulia> Mocking.convert(Expr, patch)
quote
    import A.B
    bar(f::A.B.AbstractFoo) = begin
            (##662)(f)
        end
end

The solution should be to just recursively import all the parent modules.

test/patch.jl Outdated

end

import ModA
Copy link
Contributor

Choose a reason for hiding this comment

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

For 0.7, this will need to be

if VERSION >= v"0.7.0-DEV.1877"
    import .ModA
    import .ModA: bar, ModB
else
    import ModA
    import ModA: bar, ModB
end

because of JuliaLang/julia#23579

@codecov
Copy link

codecov bot commented Jul 10, 2018

Codecov Report

Merging #52 into master will decrease coverage by 1.38%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
- Coverage   93.52%   92.14%   -1.39%     
==========================================
  Files           4        4              
  Lines         278      280       +2     
==========================================
- Hits          260      258       -2     
- Misses         18       22       +4
Impacted Files Coverage Δ
src/Mocking.jl 86.66% <88.88%> (-1.97%) ⬇️
src/expr.jl 94.25% <0%> (-2.3%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f49d4e...c745cf8. Read the comment docs.

@rofinn rofinn changed the title WIP: Handle nested modules Handle nested modules Jul 10, 2018
test/patch.jl Outdated
x::String
end

end
Copy link
Member

@omus omus Jul 10, 2018

Choose a reason for hiding this comment

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

Can we indent the submodule content for clarity?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or even just end # ModA comments

src/Mocking.jl Outdated
push!(modules, m)
m = m.args[1]
end
push!(modules, m)
Copy link
Member

Choose a reason for hiding this comment

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

Why the change?

Copy link
Member

Choose a reason for hiding this comment

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

lol, you fixed this in a very different way than I thought you would. XD

test/patch.jl Outdated
@@ -78,8 +101,17 @@ end
]
for p in patches
@test p.signature == :(f(h::Core.Int64=$RAND_EXPR(Core.Int64)))
@test p.modules == Set([:Core, RAND_MOD_EXPR])
@test p.modules == Set([:Core, :Base, RAND_MOD_EXPR])
Copy link
Member

Choose a reason for hiding this comment

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

@rofinn
Copy link
Collaborator Author

rofinn commented Jul 10, 2018

I guess we'll need to start special casing Main on 0.7 now?

@rofinn
Copy link
Collaborator Author

rofinn commented Jul 11, 2018

Yeah, I'm not sure how to make these tests work on 0.7 because:

  1. Manually, removing references to :Main doesn't work (and is ugly)
  2. Referencing relative modules doesn't seem to work inside the patch on 0.7
  3. Defining the test module inside Mocking doesn't work because we need to run Mocking.enable before loading the test module.

Maybe I'll only run the test on 0.6 for now with a comment on why.

@ararslan
Copy link
Contributor

Could you not use fullname to get the full path to a module rather than having to use a relative reference? Also import Main, while a strange thing to do, does work in 0.7, at least at the REPL.

@iamed2
Copy link
Contributor

iamed2 commented Jul 11, 2018

Also import Main, while a strange thing to do, does work in 0.7, at least at the REPL.

It works when you're in Main, like at the REPL. If you're in a package, it won't work, because each package has its own top level module (e.g., parentmodule(Compat) == Compat).

Copy link
Member

@omus omus left a comment

Choose a reason for hiding this comment

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

I'm fine with having this tested only on 0.6 for now.

@rofinn rofinn merged commit b0f5243 into master Jul 11, 2018
@ararslan ararslan deleted the rf/nested-mods branch July 11, 2018 21:25
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

Successfully merging this pull request may close these issues.

4 participants