JuliaPluto / PlutoDependencyExplorer.jl

Pluto's dependency sorting algorithm
https://plutojl.org/en/docs/plutodependencyexplorer/
MIT License
7 stars 2 forks source link

Loosen requirement that parsed code is of type Expr #8

Open JackDevine opened 4 months ago

JackDevine commented 4 months ago

I have started using the dependency explorer and the expression explorer for a project of mine and it has been quite fun.

However, I did notice a slight problem, which is that if you have a cell with contents "notebook", then updated_topology breaks.

MWE:

import PlutoDependencyExplorer as PDE
struct SimpleCell <: PDE.AbstractCell
    code
end

notebook = SimpleCell.([
    "x + y"
    "x = 1"
    "y = x + 2"
    "notebook"  # this cell breaks updated_topology
])

empty_topology = PDE.NotebookTopology{SimpleCell}();

topology = PDE.updated_topology(
    empty_topology,
    notebook, notebook;
    get_code_str = c -> c.code,
    get_code_expr = c -> Meta.parse(c.code),
)
ERROR: MethodError: no method matching PlutoDependencyExplorer.ExprAnalysisCache(::String, ::Symbol)

Closest candidates are:
  PlutoDependencyExplorer.ExprAnalysisCache(::Any, ::Any, ::Any, ::Any, ::Any)
   @ PlutoDependencyExplorer C:\Users\jackn\.julia\packages\PlutoDependencyExplorer\vDlZV\src\Topology.jl:5
  PlutoDependencyExplorer.ExprAnalysisCache(::String, ::Expr, ::ExpressionExplorer.UsingsImports, ::Bool, ::Union{Nothing, UInt64})
   @ PlutoDependencyExplorer C:\Users\jackn\.julia\packages\PlutoDependencyExplorer\vDlZV\src\Topology.jl:5
  PlutoDependencyExplorer.ExprAnalysisCache(::String, ::Expr)
   @ PlutoDependencyExplorer C:\Users\jackn\.julia\packages\PlutoDependencyExplorer\vDlZV\src\Topology.jl:12

Stacktrace:
 [1] updated_topology(old_topology::PlutoDependencyExplorer.NotebookTopology{…}, notebook_cells::Vector{…}, updated_cells::Vector{…}; get_code_str::var"#1#3", get_code_expr::var"#2#4", get_cell_disabled::Function)
   @ PlutoDependencyExplorer C:\Users\jackn\.julia\packages\PlutoDependencyExplorer\vDlZV\src\TopologyUpdate.jl:43
 [2] top-level scope
   @ REPL[7]:1
Some type information was truncated. Use `show(err)` to see complete types.
JackDevine commented 4 months ago

OK, I see. So the problem occurs when the result of Meta.parse(c.code) is not an Expr, so changing get_code_expr to get_code_expr = c -> let c = Meta.parse(c.code); c isa Expr ? c : Expr(c); end fixes the problem.

I am happy for this to be closed, although, it might be worthwhile updating the README/documentation?

fonsp commented 4 months ago

Haha that sounded like a fun issue where variables are not allowed to be called notebook!

Yes, let's try to fix or document this? How do you feel about drafting a PR, either with a fix or with updated docs about this limitation? The benefit of splitting PDE into a separate package is that it should be easier to contribute!

For context, Pluto always wraps your code in an expressions like Expr(:toplevel, linenumbernode, yourcode). This means that the cell code is always of type Expr, whereas Meta.parse can return many types, like Expr, Symbol, Bool and more. But when PDE is used outside of Pluto, the assumption that the parsed code is an Expr is not so nice.

julia> nb = Pluto.Notebook([Pluto.Cell("notebook")]);

julia> top = Pluto.updated_topology(nb.topology, nb, nb.cells);

julia> top.codes[only(nb.cells)].parsedcode
:($(Expr(:toplevel, :(#= /Users/fons/.julia/pluto_notebooks/Mini conjecture 66.jl#==#216dca0a-d6ee-11ee-3a06-37883ba6ac2c:1 =#), :notebook)))
fonsp commented 4 months ago

Btw Expr(symbol) is probably not what you want, you could use Expr(:block, symbol).

So: c isa Expr ? c : Expr(:block, c).

fonsp commented 4 months ago

Also I would love to hear more about your project! πŸ’› It took a lot of work to factor out PDE into its own package so I'm really happy to hear that it's useful to you!

JackDevine commented 4 months ago

Thanks for the quick response and all of your input @fonsp !

I'd be more than happy to contribute a fix or updated documentation πŸ˜„. Right now I am in favor of making a code fix, rather than a doc fix. I will think about it some more and let you know.

I will have to set aside a bit of time to get everything PR ready, so it will probably take about 1-2 weeks. Is that an OK timeline?

Also I would love to hear more about your project! πŸ’› It took a lot of work to factor out PDE into its own package so I'm really happy to hear that it's useful to you!

Yes, I am very grateful for the work you have done here (and throughout the PlutoVerse), PDE is just one of the things that I find useful.

The project that I am working on is a variable explorer that shows the variables in your session along with their values and dependencies/dependents. The variables are clickable through the pluto-link functionality, so you can use the variable explorer to quickly jump to the definition of any variable. Here is a really early preview:

image

There are still a lot of wrinkles to iron out, so it will take time before I could actually share any results, but it is definitely something that I have wanted for a while.

fonsp commented 4 months ago

Yes perfect, take your time!

That variable explorer looks really good!! Maybe the new API https://github.com/JuliaPluto/AbstractPlutoDingetjes.jl/pull/13 could be useful for you?

fonsp commented 4 months ago

This will be really useful for people coming from Matlab or R Studio! I often get asked "where is the variable explorer?" πŸ™

We are working on a category of featured notebooks called "Pluto for you" on https://github.com/JuliaPluto/featured , starting with https://github.com/JuliaPluto/featured/pull/54 . If this was released as a package we would really like to use it in a "Pluto for Matlab users" notebook!