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

Improving console.html #875

Closed
wants to merge 8 commits into from
Closed

Improving console.html #875

wants to merge 8 commits into from

Conversation

casatir
Copy link
Contributor

@casatir casatir commented Dec 16, 2020

console.htmlis a great tool. It's a showcase for Pyodide and the devel version allows developers to track issues and make quick tests.

Unfortunately, IMHO, it lacks some important features like:

  1. sync behavior (do not set next prompt until current command finished, e.g. with imports)
  2. full stdout/stderr support (e.g. import warnings are not visible)
  3. correct result representation with repr (type "1" and it shows 1, type globals() and it shows [object Object])

While 1. and 2. are addressed in this PR, 3. seems tricky, at least with the current setup (JQuery's terminal + copy.py interactive console + runPythonAscyn) but not impossible (see Basthon).

Getting correct representations for basic types (like strings) are addressed in this PR. Difficulty to get correct repr for complex objects comes from the fact that, when you get a Python object on the JS side (e.g. runPython[Async]'s result), it seems impossible to get it back into Python. As an illustration, compare the result of the two following code snippets:

// returns "[object Object]"
pyodide.runPython("import js ; js.window.x = {1: 2} ; repr(js.window.x)")
// returns "{1: 2}"
pyodide.runPython("x = {1: 2} ; repr(x)")

Long story short,

pyodide.runPython("repr({1: 2})")
// is not equivalent to:
pyodide.pyimport("repr")(pyodide.runPython("{1: 2}"))

Any idea to fully address 3.with the current setup?

Should I edit changelog ?

Closes #897

@rth
Copy link
Member

rth commented Dec 17, 2020

Thanks a lot for improving console.html @casatir !

For the point 3 could you open an issue about it? I think it's a general issue with py ⟷ JS conversions, possibly related to #780.

Yes, please add a changelog entry with a link to this PR.

"""
def __init__(self, flush_callback):
buffer = io.BufferedWriter(CallbackBuffer(flush_callback))
super().__init__(buffer, line_buffering=True)
Copy link
Member

@rth rth Dec 17, 2020

Choose a reason for hiding this comment

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

Could you please move these 3 classes to src/pyodide.py, keep them private (e.g. _ConsoleStdStream, _ConsoleCallbackBuffer, _Console) and add some unit tests for them in src/tests/test_pyodide.py. Eventually we might make an actual pyodide Python package with multiple modules, but for now I thing putting everything in a single file is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that this should be tested but do you think src/pyodide.py is the better place to put REPL stuff?

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise we would need to address #896 first and then put it under pyodide/console.py. Maybe that would be better indeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Answered to #896 where your proposal makes sense. However, should this be blocking for this PR?

@phorward
Copy link
Contributor

Hello @casatir, thank you for your contribution.

I'm also of the same opinion, the REPL is a very important place to start with. I've just compiled and tested your branch, it runs well, but I found about another problem:

s = input()

shows a JavaScript prompt() in 0.15.0 (anyway, I doesn't return anything), but now yields in an error

>>> s = input()
Error: Traceback (most recent call last):
  File "/lib/python3.8/site-packages/pyodide.py", line 66, in eval_code
    exec(compile(mod, "<exec>", mode="exec"), ns, ns)
  File "<exec>", line 1, in <module>
ValueError: I/O operation on closed file.

Is there a way to resolve this? Please do also update the CHANGELOG.md.

@casatir
Copy link
Contributor Author

casatir commented Dec 18, 2020

I will update the changelog and put console stuff in pyodide.py.

Are you sure about the input issue? My tests shows that it works like in 0.15.0...

@phorward
Copy link
Contributor

phorward commented Dec 18, 2020

Are you sure about the input issue? My tests shows that it works like in 0.15.0...

you're right... I've just built it again and it works. Then forget about it.

@casatir
Copy link
Contributor Author

casatir commented Dec 19, 2020

Here is an attempt to fix #897 . Seems OK but namespace is polluted by imports and variables from the REPL code. Not tab completion on filepath (not supported by Jedi from what I understand).

@rth
Copy link
Member

rth commented Dec 20, 2020

Here is an attempt to fix #897 . Seems OK but namespace is polluted by imports and variables from the REPL code. Not tab completion on filepath (not supported by Jedi from what I understand).

Great thanks! We can look into improving that situation in a separate PR.

Answered to #896 where your proposal makes sense. However, should this be blocking for this PR?

I'll try to make a PR to create that package soon, but it's not a blocker. However we need tests for those classes to merge this PR, which for now probably means putting them into src/pyodide.py.

@rth rth added this to the 0.16.0 milestone Dec 21, 2020
@rth rth mentioned this pull request Dec 21, 2020
@casatir
Copy link
Contributor Author

casatir commented Dec 22, 2020

I think I have a working console with:

  • sync behavior
  • full stdout/stderr support
  • correct result representation
  • clean namespace
  • integration in a new environment made easy
  • tests

It is fully implemented in Python and demonstrates the Pyodide ability to interpret itself.

That's a lot of changes that I'll split into several PR.

See you soon!

@hoodmane hoodmane mentioned this pull request Dec 23, 2020
@rhildred rhildred mentioned this pull request Jan 3, 2021
@rth rth modified the milestones: 0.16.0, 0.17.0 Jan 5, 2021
casatir added a commit to casatir/pyodide that referenced this pull request Jan 11, 2021
casatir added a commit to casatir/pyodide that referenced this pull request Jan 14, 2021
casatir added a commit to casatir/pyodide that referenced this pull request Jan 14, 2021
@rth
Copy link
Member

rth commented Jan 15, 2021

Should we close this PR @casatir following #1125 and given other other PRs that you were planing to open?

@casatir
Copy link
Contributor Author

casatir commented Jan 15, 2021

Should we close this PR @casatir following #1125 and given other other PRs that you were planing to open?

Yes, #1141 should implement the remaining issues exposed here (tab completion will be added in a separate PR).

@casatir casatir closed this Jan 15, 2021
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.

Autocompletion in REPL
3 participants