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

Loosen requirement that parsed code is of type Expr #8

Open
JackDevine opened this issue Feb 29, 2024 · 7 comments · May be fixed by #10
Open

Loosen requirement that parsed code is of type Expr #8

JackDevine opened this issue Feb 29, 2024 · 7 comments · May be fixed by #10

Comments

@JackDevine
Copy link

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
Copy link
Author

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
Copy link
Member

fonsp commented Feb 29, 2024

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
Copy link
Member

fonsp commented Feb 29, 2024

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

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

@fonsp fonsp changed the title A cell containing "notebook" breaks updated_topology Loosen requirement that parsed code is of type Expr Feb 29, 2024
@fonsp
Copy link
Member

fonsp commented Feb 29, 2024

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
Copy link
Author

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
Copy link
Member

fonsp commented Mar 4, 2024

Yes perfect, take your time!

That variable explorer looks really good!! Maybe the new API JuliaPluto/AbstractPlutoDingetjes.jl#13 could be useful for you?

@fonsp
Copy link
Member

fonsp commented Mar 4, 2024

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 JuliaPluto/featured#54 . If this was released as a package we would really like to use it in a "Pluto for Matlab users" notebook!

@JackDevine JackDevine linked a pull request Mar 18, 2024 that will close this issue
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

Successfully merging a pull request may close this issue.

2 participants