Merge stdin, stdout and stderr to better imitate a standard terminal #694
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The damn npm scripts are all Linux-only... It seems like I've tried everything and I still can't get the tests to run. I could try using WSL, but that's also a bit of a janky solution... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
This is a draft PR as there still are some problems yet to be resolved. See Unresolved Problems for more info.
resolves #692
resolves #693
Behaviour Comparison
Previously raised issues
input() missing from output (#693)
Old Behaviour
New Behaviour
Console Behaviour
Program output swallowed by exception (#692)
Old Behaviour
New Behaviour
Console Behaviour
Error swallowed by program output (#692)
Old Behaviour
New Behaviour
Console Behaviour
New behavioural changes
Switch input() prompt from __repr__ to __str__
Old Behaviour
New Behaviour
Console Behaviour
Raise exception if input() cannot return a str object
Old Behaviour
New Behaviour
How input's behaviour is documented
As you cannot cancel the input operation on Console, we cannot really compare our behaviour to it. Instead we must compare it to the documented behaviour. While cancelling the operation isn't documented, I felt that raising an exception was the more sane option since type checkers such as Pyright (see below) define that an input must only return an object of type
str.Implementation Details
Usually I wouldn't explain the implementation, but I felt it was necessary due to the sheer amount of changes made.
General
The separate code-error element was removed and errors were redirected to the code-output element. The differentiation between stdout and stderr was removed and the code output is now just referenced as output. A colour can now be passed when printing an output which is currently used by stderr and input().
Stderr
As stdin and stderr is now consolidated to a single place, stderr has very little custom functionality over the standard stdout.
When a stderr message is printed, the code-output element is tagged as errored by adding a custom class to it. CSS will detect this class and format the output with a red border.
The message of stderr is forwarded to output with a red colour.
Stdin/input
The digabi's input() function was reworked to be more in-line with Python's inbuilt input(). This new function will override Python's default input() using the builtins module. All imports from overriding input() are now cleaned as well.
As we override the function in the builtins module, this forbids the user from using Python's builtin input(), but I felt that this wasn't such big of a problem as long as we can stick as close as possible to Python's default input() behaviour.
The new input() function behaves correctly when translating objects into prompts by using __str__ instead of __repr__ and writes the input prompt + value onto the output as blue.
Blue was chosen as from my limited research, it seemed to be the most distinct colour compared with error's red for people with colour blindness.
The new input() also raises an error when cancelled (we detect when prompt returns null) instead of returning None incorrectly. The error message looks kind of daunting and ugly since it's propagated from JavaScript. It can be looked into whether we can directly raise (against input()'s spec) a Python exception instead if the change is necessary. I personally think that the current format is good enough to be rolled out and can be changed later if needed.
To be able to pass this input() JavaScript function into Python, a custom digabi module was necessary. This module can be used in the future to expose more JavaScript functionality to the user if needed.
While input now skips stdin, there are still situations where stdin might be used (
fileinputmodule for example). In these cases, we fall back on a basic JS prompt with a default message.Input skipping stdin means that redirecting stdin does not work for input(),
but I don't see this as a problem since that functionality has never existed in digabi and I cannot come up with a scenario where one would run into the need to redirect stdin in digabi/Abitti if it's even possible.
Unresolved Problems
Accessibility
While I tried to retain the accessibility of the code editor, I am not certain whether everything is in order since I've never really programmed anything accessibility related before and my familiarity with this codebase isn't the best.
Tests
I wasn't able to run the provided Makefile or build the source successfully (I'm on Windows and the build tools seem to be have been made primarily for Linux) which means that I was implementing these features on a broken build. Due to this, I was not able to run any tests. These tests need to be rewritten due to the changes made to the code output, but as I can't run them, it wouldn't be wise to even try to write the tests myself.
If anyone who can get a working build of this repo would be able to write these tests, it would be massively appreciated.
As I wasn't able to run the Makefile, Pyodide needs to be updated by someone else as well if a new version is available.
While I'm very certain that I have tested all possible scenarios by hand, I cannot guarantee that this implementation is 100 % stable as my build was still broken. See below for what I mean by not being certain of 100 % stability.
Unknown Error
For some reason, I get spammed with errors every time I try to resize the code editor, but I decided to ignore this as it happens to me on earlier versions of koe-ohje as well.
This might just be an issue on my side as I wasn't able to compile the source successfully.