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

Use the server session to get bond names instead of parsing the notebook code #97

Merged
merged 9 commits into from
Dec 13, 2022

Conversation

disberd
Copy link
Member

@disberd disberd commented Nov 19, 2022

The current way PlutoSliderServer finds bonds is by going through the parsed cell code and looking for the presence of the @bind variable:

function find_bound_variables!(found::Set{Symbol}, expr::Expr)
if expr.head === :macrocall &&
expr.args[1] === Symbol("@bind") &&
length(expr.args) == 4 &&
expr.args[3] isa Symbol
push!(found, expr.args[3])
find_bound_variables!(found, expr.args[4])
elseif expr.args === :quote
found
else
for a in expr.args
find_bound_variables!(found, a)
end
end
end

This unfortunately do not work for custom macros that internally create bonds.

The SliderServer already instantiate a Pluto Server session so we can directly access the bound variables that are recorded inside the PlutoRunner module. This makes it easier to track all possible bound variables without resorting to complex rules for parsing the code within the cell inputs

Discussed this approach 2 weeks ago with @fonsp.

I will fix/add test once the related Pluto PR (fonsp/Pluto.jl#2379) is merged

@disberd disberd marked this pull request as ready for review December 8, 2022 16:10
@disberd
Copy link
Member Author

disberd commented Dec 8, 2022

@fonsp this is now ready for review

Copy link
Member

@fonsp fonsp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inlcuding HypertextLiteral and running the notebook makes the tests run in 7 minutes instead of 5. Could you write a macro that does not use HypertextLiteral? Maybe a macro that just adds "123" to the bound variable name?

If that's not a good idea for some reason then the extra 2 minutes is fine 👍

@disberd
Copy link
Member Author

disberd commented Dec 13, 2022

@fonsp I did just use the example you gave me on zulip :D, didn't think about timing impact but I removed HypertextLiteral and just used Markdown now which should not impact the time

@disberd disberd requested a review from fonsp December 13, 2022 09:50
@fonsp fonsp enabled auto-merge (squash) December 13, 2022 11:44
@fonsp fonsp merged commit ddd9e72 into JuliaPluto:main Dec 13, 2022
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 this pull request may close these issues.

2 participants