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

with_terminal to capture stdout and stdin #31

Merged
merged 12 commits into from Sep 20, 2020

Conversation

j-fu
Copy link
Contributor

@j-fu j-fu commented Sep 10, 2020

Hi, here is my PR.

Here is an example gist.

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.

Thanks for writing it into a PR! We will probably use it in our class too

I have some comments:

src/CodeControl.jl Outdated Show resolved Hide resolved
src/CodeControl.jl Outdated Show resolved Hide resolved
````

"""
macro cond(run,expr)
Copy link
Member

Choose a reason for hiding this comment

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

Why create a macro that does exactly the same as if? This makes code harder to understand

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not exactly the same if I want to use this with a begin/end block.
I can have

@cond test @with_stdout  begin
     hack hack hack
end

but I must have

if test @with_stdout  begin
     hack hack hack
end end

with two ends at the end.
But I see some point in your remark (see above).

It comes down to finding the right usage patterns. I envision to explain to new uses the working of pluto before anything else and might explain things both ways. In this sense it appears to be more important to find a proper variable name instead of test than insisting on @cond.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this macro has 2 (small) advantages over if:

  1. it replaces both the if and the end, resulting in slightly less typing.
  2. it promotes code execution dependent on a checkbox (or any other bool) to an "official" Pluto usage pattern.

However, I agree that the benefit of it is not huge and therefore it is more a matter of taste. Alternatively, the if-pattern could be documented as officially recommended.

src/CodeControl.jl Outdated Show resolved Hide resolved
@j-fu
Copy link
Contributor Author

j-fu commented Sep 10, 2020

Hi updated the PR. Keeping @cond so far...

Added the @capture code I had in the PR to Suppressor (for the time being). Anyway had to modify it as it did work from the REPL and not from Pluto. So now we catch both stdout and stderr.

See https://gist.github.com/j-fu/5afae23d4c9676b20b4f0551a97dccd1 .

@j-fu
Copy link
Contributor Author

j-fu commented Sep 11, 2020

For 9cbf04e: not sure about ignoring @async ...
Edit:
Well may be I really can't:

@with_output begin
	 @warn "xxx"
	for i=1:100000
		println(i)		
		x=3
	end
end

this seems to fill up the stream buffer and getting stuck waiting for taking out some stuff.
But then fetching it all and printing it makes no sense in a notebook.
What could be reasonable here is in good Julia style something like

1
2
3
...
99998
99999
100000

@fonsp
Copy link
Member

fonsp commented Sep 20, 2020

I have made some changes:

  • use Suppressor.jl
  • function instead of macro. The reason to use a macro would be to allow global definitions inside the wrapped expression, but this was not possible with the current macro, nor with Suppressor.jl. Since there is no benefit of using a macro, we use a function instead.
  • escape HTML characters, otherwise println("</pre>") would break the terminal.
  • removed @cond for the reasons mentioned before

@fonsp fonsp changed the title Add code control (after an idea of Benjamin Lungwitz) and output grabing with_terminal to capture stdout and stdin Sep 20, 2020
@fonsp fonsp merged commit 70586ae into JuliaPluto:master Sep 20, 2020
@j-fu
Copy link
Contributor Author

j-fu commented Sep 20, 2020

... yeah the function stuff is better, I don't like macros that much anyway. Still wasn't familiar enough with the do-block syntax to get this idea.

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.

None yet

3 participants