PyScript Terminal - the latest kind #1816
Conversation
I'd love one too but I've no idea how to do that ... the terminal takes logs (print) and/or errors and I am not sure how that test would look like ... the smoke test is OK to replicate as integration but then ... what do I check? Maybe a filter of the logs would be enough, maybe not? @antocuni I also would like to know your opinion here as I am still using code to instrument Python but maybe, thanks to later hooks, I can do something else passing over a callback instead of python code? I think the |
| return _sync.pyterminal_read(prompt) | ||
|
|
||
| _py_terminal = _PyTerminal() | ||
| sys.stdout = sys.stderr = _py_terminal |
There was a problem hiding this comment.
This is going to be problematic because _PyTerminal doesn't implement the IOBase interface. I think adding isatty and flush methods would be good enough, since a lot of things ask about sys.stdout.isatty. Though for instance, pytest tries to do os.dup2(devnull, sys.stdout.fileno()) which will basically only work if you use pyodide.setStdout() (or make your own file system device). Is there a reason that setStdout isn't working for you?
There was a problem hiding this comment.
are you suggesting I should completely ditch this Python evaluation and just use pyodide to setup the terminal properly? I am OK with that, and no reasons not to do so ... which is why I've asked @antocuni what was the thing that was missing ... I still need to use the sync feature though, right?
There was a problem hiding this comment.
are you suggesting I should completely ditch this Python evaluation and just use pyodide to setup the terminal properly?
Yeah assuming I understand what you mean by that.
I still need to use the sync feature though, right?
I think so yes, but in a js function.
There was a problem hiding this comment.
yes, I think that if we can use pyodide.setStdio it would save us a lot of headaches.
This python code was a quick&dirty hack to have something kind-of-working in the worker, but then I was stuck by the impossibility to execute custom JS code in the worker (and thus I was unable to call pyodide.setdStdio).
There was a problem hiding this comment.
You can do
from pyodide_js import setStdioand then call setStdio from Python. But you'll need a JS or C function to be the handler (if you try to pass a Python function it'll throw a GilNotHeldError).
There was a problem hiding this comment.
I'll try that but I meant that this example is nowhere found online or in the docs 😅 ... or at least I couldn't find it.
There was a problem hiding this comment.
You should do something like ...
that doesn't work. The read expects a prompt to read (same as input expects a prompt) + encodeInto excepts 2 arguments so all things don't quite work as presented ... I think a live demo that shows how setStdin can be used for text would be helpful? ... I am currently keen to keep things as they are and eventually refine later, but at least I can make stdout and stderr isatty
There was a problem hiding this comment.
read expects a prompt to read
That's not correct, read should take no arguments. The way that prompt should work is as two separate steps:
writethe prompt textreadto get user data
encodeIntoexpects 2 arguments
Oops it should be encoder.encodeInto(dataStr, buffer):
let dataStr;
const encoder = new TextEncoder();
const stdin = {
read: buffer => {
if (!dataStr) {
dataStr = sync.pyterminal_read();
}
const {read, written} = encoder.encodeInto(dataStr, buffer);
dataStr = dataStr.slice(read);
return written;
},
}
There was a problem hiding this comment.
this is still the part that doesn't work:
dataStr = sync.pyterminal_read();
the read expects some value ... I might try passing an empty string but read there acts like input(prompt) and returns the text later ... that's how XTerm uses is too.
Thanks for the update though, I'll see if I can manage to make it work.
There was a problem hiding this comment.
I might have something working as expected after a bit of playing around the needed dance to have an equivalent DX #1833 if you could have a quick look at the workerReady callback maybe you can tell me if that makes sense or if it doesn't ... all I know is that it's the only one that worked to me. Thank you!
|
OK, this is ready to go ... I am only waiting to understand if |
Description
This MR brings in what agreed and discussed in #1801 (comment)
Changes
Checklist
docs/changelog.md