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

show() methods aren't dependencies #326

Open
cmcaine opened this issue Aug 29, 2020 · 6 comments
Open

show() methods aren't dependencies #326

cmcaine opened this issue Aug 29, 2020 · 6 comments
Labels
backend Concerning the julia server and runtime reactivity The Pluto programming paradigm

Comments

@cmcaine
Copy link

cmcaine commented Aug 29, 2020

show() methods aren't considered when determining which cells to re-evaluate, so you can end up with a situation like this where the rendered notebook is inconsistent with how it would look if you re-evaluated cell 2:

image

Suggested fix: maintain a dependency DAG for outputs as well as cells; outputs depend on the cell that calculated them and on the definitions of show or showerror.

So the definition of a more specific showerror method in cell 3 will cause the output of cell 2 to be re-rendered (but won't cause cell 2 to be re-evaluated).

I suggest this partly because the DAG looks like it works on symbol names rather than methods and it would be a pain to invalidate every cell any time a method is added to show or showerror or whatever.

Reproducible example:

### A Pluto.jl notebook ###
# v0.11.9

using Markdown
using InteractiveUtils

# ╔═╡ 4f2dc5b0-ea04-11ea-3f4c-073ac0ed718b
struct MyException <: Exception
	msg::String
end

# ╔═╡ a46df5e0-ea04-11ea-0700-df5e78befa62
throw(MyException("foo"))

# ╔═╡ 35271030-ea05-11ea-00e8-2328e8688cb7
Base.showerror(io::IO, e::MyException) = print(io, "MyException: ", e.msg)

# ╔═╡ 26b7b090-ea05-11ea-2edd-09565dee8c58
throw(MyException("foo"))

# ╔═╡ Cell order:
# ╠═4f2dc5b0-ea04-11ea-3f4c-073ac0ed718b
# ╠═a46df5e0-ea04-11ea-0700-df5e78befa62
# ╠═35271030-ea05-11ea-00e8-2328e8688cb7
# ╠═26b7b090-ea05-11ea-2edd-09565dee8c58
@fonsp
Copy link
Owner

fonsp commented Aug 30, 2020

Thanks! The suggested solution is too complex, but we can also just re-show every cell when a new show method is defined?

@cmcaine
Copy link
Author

cmcaine commented Aug 30, 2020

That sounds like it would have the same effect 👍

@fonsp fonsp added backend Concerning the julia server and runtime reactivity The Pluto programming paradigm labels Sep 1, 2020
@cmcaine
Copy link
Author

cmcaine commented Sep 7, 2020

I thought I might take a look at this. Are you open to a PR on it or have you already started on it or something?

Would you like to give some pointers on where to start or how you'd like it done? (optional)

@fonsp
Copy link
Owner

fonsp commented Sep 7, 2020

That would be great!

I think that you should check if any function defines a method for show or Base.show here:


and if so, call this on every cell:
function run_single!(notebook::Union{Notebook,WorkspaceManager.Workspace}, cell::Cell)
run = WorkspaceManager.eval_format_fetch_in_workspace(notebook, cell.parsedcode, cell.cell_id, ends_with_semicolon(cell.code))
cell.runtime = run.runtime
cell.output_repr = run.output_formatted[1]
cell.repr_mime = run.output_formatted[2]
cell.errored = run.errored
return run
end

except it should not run again, it should only call the output fetcher.

@cmcaine
Copy link
Author

cmcaine commented Sep 7, 2020

I'm looking at adding a test in test/React.jl, which I think is the right place?

And while there I see this line:

begin

I think you probably want let, rather than begin. begin does not introduce a new scope.

I can open a PR for that, too.

@fonsp
Copy link
Owner

fonsp commented Sep 7, 2020

Haha thanks but that's too small to spend time on

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Concerning the julia server and runtime reactivity The Pluto programming paradigm
Projects
None yet
Development

No branches or pull requests

2 participants