Add support for the Julia workspace view #1003
Conversation
|
Glad to see this feature is already being developed! This is one of the top 3 in my wish list to get our colleague to adopt Julia + VSCode as standard development environment. FY the other two being intellisense for local module on LOAD_PATH and loaded with "using" or "import"; and GUI for profiler. All these 3 features are currently supported by Juno, but we prefer VSCode's overall performance. |
|
@julia-vscode/core I'm tempted to merge an initial version of this without tree drill-down, and then add that in a later version? If we were to go with that, this is almost ready to go. We could for example ship a v0.16 that includes the inline eval results, the module selector and this PR, and all relatively soon? |
|
Yeah, let's do that. The main difficulty with the tree drill functionality is that it needs to be lazy to be useful (in case of circular refs and really big objects, both of which happen all the time). Juno in theory has the code for that, but in practice it's buggy and might mess with the GC at times, so we should be very careful about the implementation. |
|
Ha, I had hoped you had a good way to handle the lazy thing :) I've been thinking how to do this, and always ended up at "that is actually quite tricky"... We should probably look at the debug adapter protocol implementation again, that seems fairly robust and we could probably mimic that. But yes, lets do that later. |
| x==Main.vscodedisplay && continue | ||
| n==Symbol("@run") && continue | ||
| n==Symbol("@enter") && continue |
There was a problem hiding this comment.
why we don't wrap those three into _vscodeserver ? export @run, @enter; using _vscodeserver won't work ?
There was a problem hiding this comment.
Yep, good idea! That won't change the need to filter them out here, right?
Should we do this in a separate PR?
There was a problem hiding this comment.
If we wrap them in _vscodeserver, then these condition check won't be needed since you get those names only from Main
There was a problem hiding this comment.
That raises a good point, actually -- we should probably have a way to switch modules (or follow the editor module).
There was a problem hiding this comment.
yeah, while I think module switching can be done in another follow up PR.
There was a problem hiding this comment.
We should probably have a tree drill down for modules in the workspace, right? Definitely a new PR, though :)
I'll do the rearrangements that @aviatesk suggested in a new PR once this one here is merged.
pfitzseb
left a comment
pfitzseb
left a comment
There was a problem hiding this comment.
Looks good to me. Left a few comments inline.
|
|
||
| @async begin | ||
|
|
||
| @async try |
There was a problem hiding this comment.
What's the point of this? The REPL connection is effectively dead after the catch anyways, no?
There was a problem hiding this comment.
There is at least an error message shown in the REPL, otherwise the error just silently disappears. What we should really do is put a crash report into the catch block, I'll do that in a new PR once this one is merged (it requires a few more things).
|
|
||
| # Replace all top level assignments with a global top level assignment | ||
| # so that they happen, even though the code now runs inside a | ||
| # try ... finally block |
There was a problem hiding this comment.
FWIW, Juno doesn't bother with all of this and instead does something like
main_mode.on_done = REPL.respond(Base.active_repl, main_mode; pass_empty = false) do line
if !isempty(line)
quote
$evalrepl($mod, $line)
end
end
end
function evalrepl(mod, line)
try
errored = false
JSONRPC.send_notification(conn_endpoint, "repl/starteval", nothing)
with_logger(JunoProgressLogger()) do
try
line = Meta.parse(line)
catch err
errored = true
display_parse_error(stderr, err)
end
errored && return nothing
try
repleval(mod, line)
catch err
display_error(stderr, err, stacktrace(catch_backtrace()))
end
end
errored ? nothing : ans
catch err
# This is for internal errors only.
display_error(stderr, err, stacktrace(catch_backtrace()))
finally
JSONRPC.send_notification(conn_endpoint, "repl/finisheval", nothing)
end
end
I'm not sure which approach I like better though.
There was a problem hiding this comment.
I'm also unsure :) Should we merge as is for now, and then we can revisit later?
| let treeItem = new vscode.TreeItem(`${node.name}:`) | ||
| treeItem.description = node.value; | ||
| treeItem.tooltip = node.type; | ||
| treeItem.contextValue = 'globalvariable'; |
There was a problem hiding this comment.
Can we make this depend on whether it's actually possible for us to display them in VSCode? That way the button would only show up for tables and plots and stuff?
There was a problem hiding this comment.
Not really, the problem are tables: in the Queryverse world you can have a query where you can't tell whether something is a table or not until you try to iterate the rows for it, i.e. until you actually try to materialize the results of the query. So that pretty fundamentally prevents us from telling with certainty whether we should show the button or not.
We could, though, narrow things down a lot: the case I described above is rare, and in very many cases we should actually be able to tell whether we can show the value or not.
I've put this onto my list for another follow up PR, ok?
|
So one thing I think we need to address before merging would be how to handle "big" objects. |
well, I think a fairly simple solution would be enough for this PR. Implementing more complicated and fancier solution like lazy display as in Juno shouldn't hold this PR imho. |
| n_as_string=="@enter" && continue | ||
| startswith(n_as_string, "#") && continue | ||
| t = typeof(x) | ||
| value_as_string = Base.invokelatest(repr, x) |
There was a problem hiding this comment.
This should probably be show, no? Also would be best to stringlimit this, as @aviatesk mentioned below.
There was a problem hiding this comment.
Or maybe just wrap
into a function and reuse that.
There was a problem hiding this comment.
This is not related to this PR, but strlimit already handles isvalid character case, so we don't need outer isvalid filtering, right ?
There was a problem hiding this comment.
Yeah, I just mindlessly copy-pasted code there ;)
There was a problem hiding this comment.
I had used repr here because that is what we use for the debugger. But I guess the situation here is different: for the debugger we ideally want the ability to round-trip things (because users can modify the value of a var in the tree view that shows vars), whereas here that is not a concern. Although: we could of course in the future add an ability to change variable values in the workspace view? In any case, I've changed it to show for now, we can iterate in future PRs.
|
|
||
| JSONRPC.send_success_response(conn_endpoint, msg, [Dict{String,String}("name"=>i.name, "type"=>i.type, "value"=>i.value) for i in vars]) | ||
| elseif msg["method"] == "repl/showingrid" | ||
| var = Core.eval(Main, Meta.parse(msg["params"])) |
There was a problem hiding this comment.
This should probably be written as
var = try
getfield(Main, Symbol(msg["params"]))
catch err
Base.display_error(err, catch_backtrace())
return
end
try
Base.invokelatest(internal_vscodedisplay, var)
catch err
Base.display_error(err, catch_backtrace())
end
or something similar.
|
Alright, I think I addressed the most urgent comments. In general, what I quite often do with feedback on these large PRs is that for things that are super critical, I make a note and then open new smaller PRs right after this PR here has been merged. I generally find that an easier workflow: the less I change in response to your comments here, the less churn we have and the faster we can merge this PR here :) Hope that is ok, I will do those (small) follow up PRs before we ship. |
|
Yup, sounds good. |
|
What is the status of the debugger for VScode for Julia? It is working great except my debug windows for variables, stack... are blank when I run debug on Julia code. Is this because of a bug or it is me? Here is what I have installed:
In the attachment/screenshot you can see that I have not made any changes to the config - is this part of my problem getting this to work properly? |
|
@tj10ai can you open a new issue for distinct problems? This was a PR for the workspace view. |
Fixes #516.
@pfitzseb told me how we can have a better hook into the REPL, so this PR brings this feature back.
And it is improved :) Better UI for vars, a button to open things like tables in the grid viewer or plots in the plot pane directly from the workspace.
Open issues:
@runand@enterin the var listinternal_vscodedisplayworks with anything we throw at it (in particular vectors and matrices) EDIT: Solved in Rearrange code and add support for vector and matrix to grid #1258.