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

Add REPL plugin hooks; Add output, output-mode, stderr attributes #1106

Merged
merged 29 commits into from Mar 23, 2023

Conversation

JeffersGlass
Copy link
Member

@JeffersGlass JeffersGlass commented Jan 11, 2023

This PR does four major things

  • Add two new plugin lifecycle methods, beforePyReplExec() and afterPyReplExec(), which which are called immediately before and after a py-repl tag is evaluated, respectively.
  • Restore the output attribute of <py-repl> tags. If provided, the Element with the ID specified by output is used as:
    • The location to send writes to sys.stdout
    • The location where the result of evaluating the REPL (if any) is display()'d
  • Restore the output-mode attribute of <py-repl> tags. If the output-mode == 'append', the contents of the output element (either specified by the output attribute or the default location as the next sibling of the REPL) are not cleared before writing new contents. This is not typical "REPL-like" behavior, but may be wanted if the user is already writing the REPL output somewhere else. (Removed in #881.)
  • Add a stderr attribute for <py-repl> tags. If provided, the Element with the ID specified by the output is used as the location to send writes to sys.stderr

All writes to the above locations (using output or stderr attributes) are in addition to writing to the py-temrinal / ioMultiplexer.


This PR is a ready for review. The task list was:

  • Add beforePyReplExec(), afterPyReplExec() to to plugins
  • Expand StdioDirector plugin to accommodate new <py-repl> attributes
  • Integration Tests:
    • New integration tests for new attributes
    • Update old plugin integration tests with new attributes
  • Documentation
  • Changelog Entries
  • Review for completeness and correctness

Resolves #988, Fixes #1070, Resolves #975, Resolves #1074

@JeffersGlass
Copy link
Member Author

Apologies that this has stayed in draft for so long - I hope to get this completed and ready to review sometime this week.

@JeffersGlass JeffersGlass added tag: repl tag: plugins Related to the infrastructure of how plugins work, or to specific plugins. labels Jan 27, 2023
@JeffersGlass JeffersGlass self-assigned this Jan 27, 2023
@JeffersGlass JeffersGlass reopened this Feb 6, 2023
@JeffersGlass JeffersGlass marked this pull request as ready for review February 6, 2023 15:11
@JeffersGlass JeffersGlass changed the title Add REPL plugin hooks; Add output, output-mode, stderr attributes [WIP] Add REPL plugin hooks; Add output, output-mode, stderr attributes Feb 6, 2023
Copy link
Contributor

@FabioRosado FabioRosado left a comment

Choose a reason for hiding this comment

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

Apologies for the LOOOOONG delay in reviewing this PR Jeff! Left some comments/suggestions and approving 😄

pyscriptjs/src/components/pyrepl.ts Outdated Show resolved Hide resolved
pyscriptjs/src/main.ts Outdated Show resolved Hide resolved
pyscriptjs/src/plugin.ts Outdated Show resolved Hide resolved
pyscriptjs/src/plugin.ts Outdated Show resolved Hide resolved
pyscriptjs/src/plugins/stdiodirector.ts Show resolved Hide resolved
Copy link
Member

@marimeireles marimeireles left a comment

Choose a reason for hiding this comment

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

Just some minor comments here
🚀
Great work!

docs/reference/elements/py-repl.md Show resolved Hide resolved
pyscriptjs/src/plugins/stdiodirector.ts Outdated Show resolved Hide resolved
pyscriptjs/src/plugins/stdiodirector.ts Outdated Show resolved Hide resolved
pyscriptjs/src/plugins/stdiodirector.ts Outdated Show resolved Hide resolved
@marimeireles
Copy link
Member

marimeireles commented Mar 7, 2023

Oh no, a lot has changed 😕
@JeffersGlass lmk if you want I can take this over and rebase after I finish the event stuff.

@JeffersGlass
Copy link
Member Author

@marimeireles @FabioRosado Finally got around to addressing the comments and merging from main! Would you mind doing a re-review?

For some reason, the Circle CI tests are stuck on a mergability check from a much earlier commit, and won't run the most recent tests at all. Any ideas how to clear those? Everything's merged and passing tests (locally) as of right now, for what it's worth. If they won't clear, I'll open a fresh PR with this branch again I guess.

@marimeireles
Copy link
Member

No idea why @JeffersGlass, sorry I missed your comment here.
To me it shows a bunch of unresolved conflicts.
Guess I'll give it a try on these :)

@marimeireles
Copy link
Member

marimeireles commented Mar 22, 2023

Ok, I've rebased 👍
Got quite a few failing tests.

tests/integration/test_00_support.py ...............                                   [  8%]
tests/integration/test_01_basic.py ................s.                                  [ 18%]
tests/integration/test_02_display.py .......................                           [ 31%]
tests/integration/test_03_element.py ...............                                   [ 39%]
tests/integration/test_async.py .......                                                [ 43%]
tests/integration/test_importmap.py xX                                                 [ 44%]
tests/integration/test_interpreter.py ....                                             [ 47%]
tests/integration/test_plugins.py ...F.......                                          [ 53%]
tests/integration/test_py_config.py ..FF........                                       [ 60%]
tests/integration/test_py_repl.py ............F..F.FFFFFF                              [ 73%]
tests/integration/test_py_terminal.py .......                                          [ 76%]
tests/integration/test_splashscreen.py ...s..                                          [ 80%]
tests/integration/test_stdio_handling.py ...........                                   [ 86%]
tests/integration/test_style.py ..                                                     [ 87%]
tests/integration/test_warnings_and_banners.py .                                       [ 88%]
tests/integration/test_zz_examples.py F.....s............x.                            [100%]

Will make sure I've merged everything correctly cause it was a bunch of files and then force push it here.

@JeffersGlass
Copy link
Member Author

Got quite a few failing tests.

This is very odd to me - I merged from main yesterday, all tests passing locally, and pushed. I think the merge conflicts here are potentially an old circle ci artifact that won’t close? Going to try opening a fresh PR from the branch and see what happens…

@marimeireles marimeireles mentioned this pull request Mar 22, 2023
3 tasks
@marimeireles
Copy link
Member

Hey @JeffersGlass, that would also work!
I've made a fresh PR here if you think it's easier and if it looks correct for you: #1301
But if the tests were passing locally to you then I might have merged something wrong... I was checking and it seems fine but will wait for you to come back to me so we don't dup. work.

@marimeireles
Copy link
Member

Ok great :)
This looks great!
I'll close my dupped PR and we continue from here =)

@JeffersGlass
Copy link
Member Author

JeffersGlass commented Mar 22, 2023

Sounds good @marimeireles! I suspect I had messed up the inital merge/rebase, but this one is at least free of merge conflicts now as well. Looks like tests are passing here, but linting is not.

@marimeireles
Copy link
Member

You can also see the local linting issues by running pre-commit run --all-files.

Copy link
Member

@marimeireles marimeireles left a comment

Choose a reason for hiding this comment

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

Went over it again and it looks good!
Thanks for coming back to this one @JeffersGlass, great work! 😊

@marimeireles marimeireles self-requested a review March 22, 2023 17:53
Copy link
Member

@marimeireles marimeireles left a comment

Choose a reason for hiding this comment

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

Actually, there's one problem.. Let me check.
I'm looking into: https://github.com/pyscript/pyscript/actions/runs/4492024745/jobs/7901322111?pr=1106
That was introduced by me as I fixed linting.

Copy link
Member

@marimeireles marimeireles left a comment

Choose a reason for hiding this comment

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

Okay, this is good now!

@JeffersGlass
Copy link
Member Author

Amazing! I'll get it merged later today!

@JeffersGlass JeffersGlass merged commit ef793ae into pyscript:main Mar 23, 2023
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tag: plugins Related to the infrastructure of how plugins work, or to specific plugins. tag: repl
Projects
None yet
3 participants