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

Progress macro does nothing if Juno is not loaded #131

Closed
MikeInnes opened this issue May 1, 2018 · 7 comments
Closed

Progress macro does nothing if Juno is not loaded #131

MikeInnes opened this issue May 1, 2018 · 7 comments

Comments

@MikeInnes
Copy link
Member

julia> using Juno, MacroTools

julia> @expand @progress for i = 1:10 sleep(1) end
:(for i = 1:10 # REPL[3], line 1:
        sleep(1)
    end)

I don't know when this changed, but this should lower to code that does the right thing when Juno is loaded (presumably the lower level progress API should work and just be a no-op). Otherwise, @progress cannot be used inside packages.

@pfitzseb
Copy link
Member

pfitzseb commented May 1, 2018

That changed in #95.

We'd need to change this to something like

function _progress(name, ex)
  quote
    if isactive()
      Atom.Progress._progress($name, $(esc(ex)))
    else
      $(esc(ex))
    end
  end
end

but I'm not sure how to pass $(esc(ex)) as an expression to Atom.Progress._progress.

@MikeInnes
Copy link
Member Author

MikeInnes commented May 1, 2018

I think you want

function _progress(name, ex)
  quote
    if isactive()
      $(Atom.Progress._progress(name, ex))
    else
      $(esc(ex))
    end
  end
end

It'd also work to just always inject the monitoring (looks like the progress api is always guarded by isactive); I guess the downside there is a small perf hit, but it seems a little weird to make that Juno-dependent anyway.

@pfitzseb
Copy link
Member

pfitzseb commented May 1, 2018

That's problematic because of

julia> @progress for i = 1:10 sleep(1) end
ERROR: UndefVarError: Atom not defined

when Atom isn't loaded as well...

@MikeInnes
Copy link
Member Author

Is there are particular reason for the macro expansion to be in Atom though? It seems like it's just lowering straight to the API defined in Juno anyway, right?

@pfitzseb
Copy link
Member

pfitzseb commented May 3, 2018

IIRC that's to avoid having Juno.jl depend on MacroTools (used here).

@pfitzseb
Copy link
Member

"Fixed" with 40dc676.

@pfitzseb
Copy link
Member

Actually fixed in latest Juno.jl/Atom.jl release.

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

2 participants