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

fixes for 0.7 #671

Merged
merged 20 commits into from Jul 6, 2018

Conversation

@fredrikekre
Copy link
Member

commented Jul 5, 2018

This is #623 + #667 + two additional fixes. Closes #661, #654, #637.

This PR works locally on both 0.7 and 0.6 for me, would be great to get IJulia to work properly on 0.7!

@fredrikekre fredrikekre force-pushed the fe/nightly branch from 4726105 to 10ddc7c Jul 5, 2018
@@ -1,6 +1,9 @@
import Base: display, redisplay

immutable InlineDisplay <: Display end
@static if !isdefined(Base, :AbstractDisplay) # remove when Compat.jl#482 is merged and tagged

This comment has been minimized.

Copy link
@stevengj

stevengj Jul 5, 2018

Member

Shouldn't this be:

if !isdefined(Base, :AbstractDisplay) && !isdefined(Compat, :AbstractDisplay)
  ...

(No need for @static in global scope unless the expressions have ccall or macros...)

This comment has been minimized.

Copy link
@stevengj

stevengj Jul 5, 2018

Member

Actually, this if statement can be deleted completely as the AbstractDisplay definition was tagged in Compat v0.52.0

@stevengj

This comment has been minimized.

Copy link
Member

commented Jul 5, 2018

Great, thanks for working on this! Sorry I haven't had time to bang on it.

Do things like println now work in both 0.6 and 0.7?

@fredrikekre

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2018

Pushed one more deprecation fix and updated to use Compat.AbstractDisplay directly.

Do things like println now work in both 0.6 and 0.7?

Yes! 🎉

logger = Base.CoreLogging._global_logstate.logger

# Override logging if it's pointing at the default stderr
if logger.stream == Base.stderr

This comment has been minimized.

Copy link
@fredrikekre

fredrikekre Jul 6, 2018

Author Member

Actually, there is something bugged here; this is only true on the first call, don't we need to reset it somewhere @staticfloat ? On this branch; with a @show logger.stream == Base.stderr inserted:

julia> using IJulia

julia> mktemp() do path, io
           redirect_stderr(IJulia.IJuliaStdio(io, "stderr")) do
               @warn("warn")
           end
           flush(io)
           seek(io, 0)
           @show read(io, String)
       end
logger.stream == Base.stderr = true
read(io, String) = "\e[33m\e[1m┌ \e[22m\e[39m\e[33m\e[1mWarning: \e[22m\e[39mwarn\n\e[33m\e[1m└ \e[22m\e[39m\e[90m@ Main REPL[2]:3\e[39m\n"
"\e[33m\e[1m┌ \e[22m\e[39m\e[33m\e[1mWarning: \e[22m\e[39mwarn\n\e[33m\e[1m└ \e[22m\e[39m\e[90m@ Main REPL[2]:3\e[39m\n"

julia> mktemp() do path, io
           redirect_stderr(IJulia.IJuliaStdio(io, "stderr")) do
               @warn("warn")
           end
           flush(io)
           seek(io, 0)
           @show read(io, String)
       end
logger.stream == Base.stderr = false
read(io, String) = ""

This comment has been minimized.

Copy link
@fredrikekre

fredrikekre Jul 6, 2018

Author Member

Perhaps it does not matter; this seems to be called only once per session anyways and it works in a notebook
warning

This comment has been minimized.

Copy link
@fredrikekre

fredrikekre Jul 6, 2018

Author Member

Would it not be simpler to just replace the global logger instead of checking the internal state?

@fredrikekre

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2018

I updated the logging redirect, now things seem to work:
working

@@ -138,7 +138,7 @@ function complete_request(socket, msg)

codestart = find_parsestart(code, cursorpos)
comps, positions = Base.REPLCompletions.completions(code[codestart:end], cursorpos-codestart+1)

This comment has been minimized.

Copy link
@stevengj

stevengj Jul 6, 2018

Member

Probably need:

if VERSION < v"0.7.0-DEV.3500" # julia#25544
    import Base: REPLCompletions
else
    import REPL: REPLCompletions
end

above so that we can just call this as REPLCompletions.completions with no deprecation warning.

This comment has been minimized.

Copy link
@fredrikekre

fredrikekre Jul 6, 2018

Author Member

Thanks, there was a similar if VERSION check at the top so I added the imports there.

@fredrikekre fredrikekre force-pushed the fe/nightly branch from f526e35 to b444612 Jul 6, 2018
@fredrikekre

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2018

While trying the tab-completion for deprecations I got this

BoundsError: attempt to access 0-element Array{Int64,1} at index [0]

Seems to be a bug here

IJulia.jl/src/handlers.jl

Lines 144 to 145 in 90b0165

elseif isempty(positions) # true if comps to be inserted without replacement
cursor_start = (cursor_end = ind2chr(msg, code, last(positions)))
where we try to extract the last element although we just made sure it's empty.

@stevengj

This comment has been minimized.

Copy link
Member

commented Jul 6, 2018

Maybe should be cursor_start = cursor_end = cursor_chr, i.e. just merge it with the previous if clause to make it if isempty(comps) || isempty(positions)

(Or maybe we should use first(positions) instead of last? But probably there isn't any case where tab completion with an empty positions range wants to insert text at someplace other than the cursor location?)

comps, positions = Base.REPLCompletions.completions(code[codestart:end], cursorpos-codestart+1)
positions += codestart-1
comps, positions = REPLCompletions.completions(code[codestart:end], cursorpos-codestart+1)
positions = positions .+ codestart-1

This comment has been minimized.

Copy link
@fredrikekre

fredrikekre Jul 6, 2018

Author Member

Nevermind, it is this fix that is missing a . on the - and the range turned into an array. Of course last works for an empty range.

@fredrikekre fredrikekre force-pushed the fe/nightly branch from b444612 to d1caa23 Jul 6, 2018
comps, positions = Base.REPLCompletions.completions(code[codestart:end], cursorpos-codestart+1)
positions += codestart-1
comps, positions = REPLCompletions.completions(code[codestart:end], cursorpos-codestart+1)
positions = positions .+ codestart .- 1

This comment has been minimized.

Copy link
@fredrikekre

fredrikekre Jul 6, 2018

Author Member

Hmm, still not correct; this returns a Vector on Julia 0.6.

@fredrikekre fredrikekre force-pushed the fe/nightly branch from d1caa23 to 57524f9 Jul 6, 2018
fredrikekre added 3 commits Jul 6, 2018
 - warn() -> at-warn
 - emtpy try-catch
 - remove_destination -> force
 - start -> firstindex
 - endof -> lastindex
 - broadcast +
@fredrikekre fredrikekre force-pushed the fe/nightly branch from 57524f9 to 0d07dbd Jul 6, 2018
@stevengj

This comment has been minimized.

Copy link
Member

commented Jul 6, 2018

LGTM, ready to merge?

@@ -198,7 +202,7 @@ function clear_history(indices)
end

# since a range could be huge, intersect it with 1:n first
clear_history{T<:Integer}(r::AbstractRange{T}) =
clear_history(r::AbstractRange{T}) where {T<:Integer} =

This comment has been minimized.

Copy link
@stevengj

stevengj Jul 6, 2018

Member

Can just be clear_history(r::AbstractRange{<:Integer}) since we don't use T.

@@ -58,30 +56,50 @@ const displayqueue = Any[]

# remove x from the display queue
function undisplay(x)
i = findfirst(equalto(x), displayqueue)
if i > 0
i = findfirst(isequal(x), displayqueue)

This comment has been minimized.

Copy link
@stevengj

stevengj Jul 6, 2018

Member

Shouldn't this be Compat.findfirst to get the version that returns nothing on 0.6? Then you can drop the i > 0 check.

function show_bt(io::IO, top_func::Symbol, t, set)
# follow PR #17570 code in removing top_func from backtrace
eval_ind = findlast(addr->Base.REPL.ip_matches_func(addr, top_func), t)
eval_ind != 0 && (t = t[1:eval_ind-1])
eval_ind = findlast(addr->ip_matches_func(addr, top_func), t)

This comment has been minimized.

Copy link
@stevengj

stevengj Jul 6, 2018

Member

Similar to above, use Compat.findlast and then you can skip the eval_ind != 0 check.

@stevengj

This comment has been minimized.

Copy link
Member

commented Jul 6, 2018

I'm just going to merge this as-is so that people can use IJulia with 0.7, and then add more fixes in another PR.

@stevengj stevengj merged commit 533ca5d into master Jul 6, 2018
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@stevengj stevengj deleted the fe/nightly branch Jul 6, 2018
@stevengj stevengj referenced this pull request Jul 6, 2018
@fredrikekre

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2018

Thanks! I have to admit that I did not review the changes from #623 myself.

@cstjean cstjean referenced this pull request Sep 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.