Skip to content

PyTerminal: use Pyodide instead of Python #1833

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

Merged
merged 1 commit into from
Nov 3, 2023
Merged

PyTerminal: use Pyodide instead of Python #1833

merged 1 commit into from
Nov 3, 2023

Conversation

WebReflection
Copy link
Contributor

Description

This MR specifically addresses this comment and while everything seems to be fine I'd love to have @hoodmane or @antocuni reading the changes and tell me if this is the best way forward, thank you!

Changes

  • drop the Python builtins.input = ... workaround and use Pyodide directly
  • update the smoke-test page to be sure everything is fine

Checklist

  • All tests pass locally
  • I have updated docs/changelog.md
  • I have created documentation for this(if applicable)

@antocuni
Copy link
Contributor

antocuni commented Nov 2, 2023

It seems good to me. What I expect is that now os.write(1, "hello\n") and os.write(2, "world\n") should go to the terminal, while previously they were not redirected.
Maybe it's worth adding an integration test?

@WebReflection WebReflection marked this pull request as ready for review November 3, 2023 14:31
@WebReflection
Copy link
Contributor Author

@antocuni

Maybe it's worth adding an integration test?

added a test and also witnessed via quick smoke test everything indeed works as expected ... ready for review!

Copy link
Contributor

@antocuni antocuni left a comment

Choose a reason for hiding this comment

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

wonderful 😍

@WebReflection WebReflection merged commit 3e2a67d into pyscript:main Nov 3, 2023
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.

2 participants