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

Allow Revise to be loaded in -e/E arguments #349

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mortenpi
Copy link
Contributor

@mortenpi mortenpi commented Sep 1, 2019

@timholy Did you mean like this?

I have been running Revise with essentially this patch (sans the Base.isinteractive check) for quite a while now and everything has been working fine.

At some point, I did see an issue when I would get some warnings with scripts (i.e. running julia script.jl). I don't remember exactly, but I think it was about stealing the REPL when there isn't one when I had using Revise in the startup file. But I wasn't able to reproduce that anymore. And hopefully the Base.isinteractive check will get around that?

As far as I can tell, this definitely fixes #202, i.e. julia -i -e 'using Revise; includet("script.jl")' now works as expected.

@mortenpi
Copy link
Contributor Author

mortenpi commented Sep 1, 2019

Also, with this patch, I think the recommended way to load Revise in the startup.jl file would become just using Revise. If I have this

atreplinit() do repl
    try
        @eval using Revise
        @async Revise.wait_steal_repl_backend()
    catch
    end
end

then the following happens

$ julia -i -e'includet("foo.jl")'
ERROR: UndefVarError: includet not defined
Stacktrace:
 [1] top-level scope at none:1

But if you just have using Revise, then julia -i -e'includet("foo.jl")' works fine. And the definitions in foo.jl will be kept up to date in the REPL session.

@mortenpi
Copy link
Contributor Author

mortenpi commented Sep 2, 2019

Ah, the warning showed up. It was the same one that @mlhetland mentioned in #202 (comment). I had using Revise in startup.jl and there was some compilation necessary, so this happened:

$ julia -i --project analysis/v4.jl
[ Info: Recompiling stale cache file /home/mortenpi/.julia/compiled/v1.2/DataFrames/AR9oZ.ji for DataFrames [a93c6f00-e57d-5684-b7b6-d8193f3e46c0]
┌ Warning: REPL initialization failed, Revise is not in automatic mode. Call `revise()` manually.
└ @ Revise ~/.julia/dev/Revise/src/Revise.jl:986

@timholy
Copy link
Owner

timholy commented Sep 3, 2019

Can we wrap this in an atreplinit?That might solve the "how long a delay?" issue.

@codecov
Copy link

codecov bot commented Sep 3, 2019

Codecov Report

Merging #349 into master will decrease coverage by 0.04%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #349      +/-   ##
==========================================
- Coverage   76.88%   76.83%   -0.05%     
==========================================
  Files          11       11              
  Lines        1168     1170       +2     
==========================================
+ Hits          898      899       +1     
- Misses        270      271       +1
Impacted Files Coverage Δ
src/Revise.jl 72.24% <50%> (-0.11%) ⬇️

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 bd7d161...fcc2129. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Sep 3, 2019

Codecov Report

Merging #349 into master will decrease coverage by 0.04%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #349      +/-   ##
==========================================
- Coverage   76.88%   76.83%   -0.05%     
==========================================
  Files          11       11              
  Lines        1168     1170       +2     
==========================================
+ Hits          898      899       +1     
- Misses        270      271       +1
Impacted Files Coverage Δ
src/Revise.jl 72.24% <50%> (-0.11%) ⬇️

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 bd7d161...fcc2129. Read the comment docs.

@mortenpi
Copy link
Contributor Author

mortenpi commented Sep 3, 2019

Ok, I've tested this wrapped version and it seems to work great. The warning is gone and loading Revise in either startup.jl or -e/E still works.

Are there any tests that should/could be added for this?

@timholy
Copy link
Owner

timholy commented Sep 4, 2019

Are there any tests that should/could be added for this?

Yes please. I suspect the best approach would be in the .travis.yml file. You'll note there are already some tests with different startup options, this would be kind of the same thing.

I haven't yet played with this myself, but any preliminary thoughts about whether this affects people who have been adding the recommended lines to their startup.jl files?

@timholy timholy added the needs tests PR needs tests before it can be merged label Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests PR needs tests before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revise loaded in -E/-e for -i not updating definitions
2 participants