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

Bug: Syntax error handling in console.html #1286

Closed
hoodmane opened this issue Feb 25, 2021 · 20 comments
Closed

Bug: Syntax error handling in console.html #1286

hoodmane opened this issue Feb 25, 2021 · 20 comments
Labels
bug Something isn't working

Comments

@hoodmane
Copy link
Member

Syntax errors in console.html are not printed immediately, only after the next command is submitted. E.g., type 1+<ENTER> into console.html.

I think this was introduced in #1125. I investigated but I can't figure out why it's happening. Any ideas @casatir?

@rth rth added the bug Something isn't working label Feb 25, 2021
@casatir
Copy link
Contributor

casatir commented Feb 25, 2021

Thanks @hoodmane for catching this. I'll investigate.

@casatir
Copy link
Contributor

casatir commented Feb 25, 2021

I am pretty sure it was working after #1155.

@hoodmane
Copy link
Member Author

I will check (unfortunately, #1155 was before the emscripten update was merged so I have to rebuild everything).

@casatir
Copy link
Contributor

casatir commented Feb 25, 2021

Doing

pyconsole.stderr_callback = $.terminal.active().error

in JS console makes it work (with newline of course).

@casatir
Copy link
Contributor

casatir commented Feb 25, 2021

So it seems related to newline stuff since it works with

pyconsole.stderr_callback = $.terminal.active().echo

but not with

pyconsole.stderr_callback = (s) => $.terminal.active().echo(s, {newline : false})

@hoodmane
Copy link
Member Author

hoodmane commented Feb 25, 2021

pyconsole.stderr_callback = $.terminal.active().error

Does that prevent sys.except_hook from happening? I would guess that doing that would cause most errors to be printed twice?

@casatir
Copy link
Contributor

casatir commented Feb 25, 2021

This is code.py 's InteractiveConsole that manage this. sys.exepthook seems to not be called in our case since https://github.com/python/cpython/blob/f82578ace103ec977cec3424b20e0b5f19cf720e/Lib/code.py#L123

@casatir
Copy link
Contributor

casatir commented Feb 25, 2021

I suspect something like syntax error occurs when terminal is paused which echo_newline.js may dislike.

@hoodmane
Copy link
Member Author

Okay it definitely does work after #1155.

@casatir
Copy link
Contributor

casatir commented Feb 25, 2021

I think I got it. In console.html, we continuously set the prompt:

async function interpreter(command) {
    // multiline should be splitted (usefull when pasting)
    term.pause();
    for( const c of command.split('\n') ) {
        const prompt = pyconsole.push(c) ? ps2 : ps1;
        term.set_prompt(prompt);
        await pyconsole.run_complete;
    }
    term.resume();
}

which disturb echo_newline.js since it also set the prompt...

@hoodmane
Copy link
Member Author

echo_newline.js uses a really awful hack. I might try to make an improvement upstream.

@casatir
Copy link
Contributor

casatir commented Feb 25, 2021

Commenting our set_prompt makes it work (but disable ps2...) showing that this might be this.

@hoodmane
Copy link
Member Author

hoodmane commented Feb 25, 2021

It's weird actually: echo_newline.js makes a copy of the original term.set_prompt as if it's planning to monkey patch it:

        term.__set_prompt = term.set_prompt;

https://github.com/jcubic/jquery.terminal/blob/9c28d597e8295a5bd282030256a9fe1b50a371c1/js/echo_newline.js#L96

but then it never does the monkey patch. It would work much better if it monkey patched set_prompt.

@casatir
Copy link
Contributor

casatir commented Feb 25, 2021

I remember I've seen this when trying to make echo_newline.js work for #1141.

@casatir
Copy link
Contributor

casatir commented Feb 25, 2021

Partially reverting #1274 restoring my term.echo_no_newline function plus

pyconsole.stdout_callback = term.echo_no_newline;
pyconsole.stderr_callback = (s) => term.echo_no_newline(`[[;red;]${$.terminal.escape_brackets(s)}\u200B]`);

makes it work without loading echo_newline.js.

@casatir
Copy link
Contributor

casatir commented Feb 25, 2021

I can submit a PR with this correction or we can wait for an upstream fix. WDYT?

@hoodmane
Copy link
Member Author

I opened a PR: jcubic/jquery.terminal#652

@casatir
Copy link
Contributor

casatir commented Feb 25, 2021

The why it happens only for syntax errors (and not runtime errors) can be understood with this three lines:

    const prompt = pyconsole.push(c) ? ps2 : ps1;
    term.set_prompt(prompt);
    await pyconsole.run_complete;

Execution steps for runtime errors (or even code exec without any error) are:

  1. pyconsole.push(c) returns false (code is not yet executed)
  2. prompt is set to >>>
  3. code is executed (via the run_complete promise), error is printed to stderr
  4. JQuery Terminal shows it by setting the prompt.

While for syntax error, we have:

  1. pyconsole.push(c) prints syntax error to stderr (and return false)
  2. JQuery Terminal shows it by setting the prompt
  3. prompt is erased by our call to set_prompt
  4. run_complete is a dummy promise so the await has no effect.

@hoodmane
Copy link
Member Author

So as an update, my jquery terminal PR has been reviewed and has received positive feedback so it will probably be merged soon.
It should make the behavior of echo / error with newline : false completely consistent with the behavior with newline : true and fix the various issues we've had with it (any interaction with set_prompt has been removed, term.error will get the color right, etc).
jcubic/jquery.terminal#654

It looks like releases of jquery.terminal are fairly frequent (about one every two weeks recently) and the most recent release was 14 days ago so it probably won't be too long before this change is included into a release.

@hoodmane
Copy link
Member Author

Closed by #1480. jquery.terminal just released a version with my PR included today!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants