Prompt IO Built-in Method #1688

robertDurst · 2023-04-01T18:49:08Z

Prompt IO Built-in Method

Adherence to Pyret's Philosophy and Core Values

Reading from stdin is a fundamental first step towards creating interactive software. I have fond memories of the earliest days of my coding career, creating simple games such as a random number guesser, tic-tac-toe, and blackjack, all of which required a stdin method to take input from the player. With the right method name, this new method should be intuitive and easy to use, a quick addition to any curious novice coder's repertoire of standard library methods.

Naming

I have chosen input since it seems the most intuitive to me. Furthermore, it is the same as Python.

However, the following names were also considered:

read
prompt

Type

While Pyret is dynamic, the method type should look like this:

INPUT: string
OUTPUT: string

Example Usage

var user_input = read("How old are you? ")
print("User is " + user_input + " years old.")

Note(s) to Reviewer(s)

** This is a WIP - my first PR here so need to do some reading about best practices, boilerplate generation, what to check in, etc. Thus, consider this PR incomplete and not ready for review until it is no longer a draft. **

package-lock.json

jpolitz · 2023-04-05T04:06:55Z

Good job finding your way around the codebase!

The main thing I'd suggest is not putting this in runtime directly, but rather in a library in trove. You can look at filelib.js for inspiration (https://github.com/brownplt/pyret-lang/blob/horizon/src/js/trove/filelib.js).

It uses the nativeRequires key of the module object to refer to fs. You can use that to refer to npm-installed modules.

I'd also consider implementing this only with node builtins. It's useful to keep the dependencies somewhat low because they all need to be (potentially) browserified, etc, and matching an external library vs. a node builtin is often more difficult.

That said, if readline-sync is getting us something great, maybe worth it (we do have libraries for s-expressions, sha256, etc that we use). What is readline-sync buying us?

robertDurst · 2023-04-07T23:02:06Z

@jpolitz working on this slowly here and there, so will ping and mark as ready for review when it is time. Thanks for the response!

The main thing I'd suggest is not putting this in runtime directly, but rather in a library in trove.

Thanks for pointing out the move to a library in trove. Was thinking an IO lib? Need to look closer and consider what might be a good module/lib to place it in.

I'd also consider implementing this only with node builtins.

100% agree. I have an aversion to just adding libraries for no reason - day job is in Ruby.... so the "there is a gem for that" mentality irks me 😅 . Something like readline-sync ** might ** get us a fancier version which handles certain input types more gracefully and is OS agnostic. That being said, I saw a few of these libs have gone on to be deprecated and furthermore, the node folks don't believe it is worth putting in std lib since it's "so easy" to implement with core node js. Thus, depending on the requirements here, I think it is completely do-able to implement with node builtins. The latest version of the code gets us a bit closer to this.

Plan to get this into a reviewable state is to add some tests and read the source of some of these node packages and see what's going on under the hood.

robertDurst · 2023-04-10T20:52:40Z

@jpolitz afaik, there is not a whole lot of mocking capabilities for Pyret. I am digging through existing tests, but not 100% sure how to write a test to ensure the prompt method works as intended; I think the most useful test would ensure that it returns, it returns what the user inputs, it performs on a variety of OS's, and then it performs special behaviors depending on input (don't plan on any of those, but robust solutions tend to allow things like handling Ctrl-D, up arrow, etc.)

I am a software engineer by day and a PR would (and should?) never be merged w/o tests, however I am struggling to figure out the best way to test this code. Let me know your thoughts.

In the meantime I will keep digging.

bennn · 2023-06-13T00:27:33Z

@jpolitz @blerner this looks good to me. What do you think? Is it okay to keep the one-off Makefile test for this?

src/js/trove/global.js

src/js/base/runtime.js

blerner · 2023-06-13T01:42:08Z

src/js/trove/iolib.js

+    nativeRequires: ["fs"],
+    theModule: function(RUNTIME, NAMESPACE, uri, fs) {
+      // https://github.com/nodejs/node/issues/28243#issuecomment-502402453
+      function PromptIO(msg) {


According to the original comment, you were considering input rather than prompt. Wouldn't it be more orthogonal to implement input, and then implement prompt as a combination of print and input?

I'm not sure I like the synchronous nature of this implementation. Since Pyret has the ability to pause the Pyret stack while an async operation occurs, and then resume the Pyret code with the results of that stack, I think it'd probably be cleaner to implement this using async-IO and pausing/resuming Pyret. @jpolitz what's your take?

src/js/trove/iolib.js

Makefile

src/js/trove/iolib.js

examples/ahoy-right-back.arr

src/js/trove/iolib.js

blerner · 2023-06-13T11:19:43Z

src/js/trove/iolib.js

+            }).then(result => { 
+              restarter.resume(RUNTIME.makeString(result));
+            }).catch(error => {
+              restarter.resume(RUNTIME.makeString(error));


This feels problematic to me -- it means you're suppressing the error and making both the normal case and the error case to have the same behavior, namely of returning a string.

Agreed - silently failing is terrible (found out some code wasn't running in prod once 4 months later because of a silent failure...). Will handle this more correctly.

I've updated this, but will revisit after we have consensus on a proper test strategy - see below.

src/js/trove/global.js

robertDurst · 2023-06-14T00:35:05Z

examples/guess-the-number.arr

@@ -0,0 +1,56 @@
+#lang pyret
+
+import iolib as IO


My first "real" pyret program. So there is certainly room for improvement.

robertDurst · 2023-06-14T00:36:57Z

src/js/trove/iolib.js

+    };
+
+    function wrapInputTest(testInput) {
+      return Input(mockIO = (rl) => rl.write(testInput + '\n'));


This is my least hacky attempt at a reasonable, repeatable test. That being said, baking it into the actual library like this and exposing it as a public interface does not seem right.

Maybe a test helper or a test only module?

The only solutions I have come up with thus far require interfacing with JS.

@blerner my biggest struggle here is figuring out a reasonable test strategy. What I have here certainly works and is less hacky than my first solution but still not as clean or comprehensive as what jest/mocha or any other "production" mocking library provides.

An ideal solution (with heavy influence from Ruby world) might look something like:

// set up mock before_check: stdin = mock-stdin(); // inject/queue-up/send/etc. stdin.inject("hello") end // execute test check: input() is "hello" end

Do we want to investigate this? It feels like scope creep but it also feels like a pre-requisite for this work? Thoughts?

Beyond that, thanks for the multiple rounds of reviews so far! I apologize, the PR was pretty messy to start with - thanks for your patience here, very much appreciated as I ramp up on this codebase.

Most likely, this will require writing a test harness in JS, rather than purely in Pyret. We have some tests that have been written this way, but I think they aren't quite what you want to emulate here. I'll need to talk to Joe a bit and bounce ideas together with him to see what we thing you ought to build here.

The idea of a before-check block is interesting, but definitely out of scope for now. There are a lot of gnarly questions that would need to be designed (e.g. when does stdin get "reset" in your example to the "real" stdin? or what should the order of execution be when you have where blocks inside functions that are called by before_check or other check blocks? A Pyret program weaves together testing and normal running into one source file, in ways that "real test code" typically doesn't...)

I think instead of mock-input-test for now, you probably just want to invoke your tests as exec("echo theInput | node theTest"), more or less. Something similar to https://github.com/brownplt/pyret-lang/blob/anchor/tests-new/simple-output.test.js or https://github.com/brownplt/pyret-lang/blob/anchor/tests-new/check-blocks.test.js#L13-L26, which are a test harness (not on the current #horizon branch, though!) that executes Pyret programs and checks their standard-out; you could pretty easily pipe in the necessary standard-in.

robertDurst · 2023-06-27T21:42:24Z

src/js/trove/iolib.js

+            .then(result => restarter.resume(RUNTIME.makeString(result)))
+            .catch(error => {
+              // TODO: we should probably NOT fail this hard
+              restarter.error(RUNTIME.makeString(error));


Not 100% sure how to trigger this so it fails. Started looking at mocking, unsure how that will play with the harness as is, may need to design it to be a bit more robust. Is a failure test something desired in a v1 implementation?

robertDurst · 2023-06-27T21:47:49Z

tests/io-tests/io.test.js

+
+          const executionStdin = runProcess.stdout.toString();
+          expect(executionStdin).toMatch(new RegExp(stdioExpected));
+          expect(executionStdin).toMatch(new RegExp(stdInexpected));


this is a bit weird but felt better than just "contains"

robertDurst · 2023-06-27T21:48:17Z

tests/io-tests/io.test.js

+          expect(runProcess.status).not.toEqual(SUCCESS_EXIT_CODE);
+
+          const executionStderr = runProcess.stderr.toString();
+          expect(executionStderr).toMatch(new RegExp(stderrExpected));


exact matching on error msg vs. error type feels brittle. Thoughts?

robertDurst · 2023-06-30T17:08:18Z

Failing with a weird npx jest error that didn't repro locally (node 14) until I switched to what the ci was using (node 12).

robertDurst · 2023-06-30T17:13:43Z

Looks like the example branch using npx may have bumped the ci to using node 16+? 👀
Is this something we'd consider @blerner? See here: https://github.com/brownplt/pyret-lang/blob/anchor/.travis.yml#L13

Bumped as per Slack: https://pyret.slack.com/archives/C1GUPMUV7/p1688392539306329

robertDurst · 2023-07-03T15:33:16Z

src/js/trove/iolib.js

+  provides: {
+    shorthands: { },
+    values: {
+      "prompt": ["arrow", ["String"], "String"]


Have Haskell on my mind - in Haskell world this would be something like:

prompt :: IO String -> String

IO in Haskell took me a little while to wrap my head around. Fairly confident we don't want to introduce this here.

robertDurst · 2023-07-03T21:27:20Z

tests/io-tests/io.test.js

+const fs = require('fs');
+const cp = require('child_process');
+
+const COMPILER_TIMEOUT = 60000; // ms, for each compiler run (including startup)


Really had to bump these - took a while and was hitting timeouts. Might be worth investigating?

robertDurst · 2023-07-03T23:45:50Z

@jpolitz @blerner I added a test harness. This should be good for another review - thanks for the patience here as I get comfortable with the Pyret codebase and we ideated on the testing approach!

blerner · 2023-08-01T01:41:56Z

tests/io-tests/io.test.js

+        const runProcess = cp.spawnSync("sh", [
+          "-c",
+          `echo ${stdInToInject} | node ${COMPILED_CODE_PATH}`
+        ], {stdio: 'pipe', stderr: "pipe", timeout: RUN_TIMEOUT});


I wonder if the reason you're getting such slow behavior here is because you're spawning a process with a pipe, but then never closing the pipe, and so for the empty-input case, it's waiting all the way until timeout? So instead of using echo ... | node ..., you can do spawnSync("node" [COMPILED_CODE_PATH], {input: stdInToInject, stdio: 'pipe', timeout: RUN_TIMEOUT}), and just read the results from stdout and stderr as before -- this might close stdin much sooner?

It seems just removing stdin: 'pipe' might have really sped things up.

However, that makes no sense...

The options.stdio option is used to configure the pipes that are established between the parent and child process. By default, the child's stdin, stdout, and stderr are redirected to corresponding subprocess.stdin, subprocess.stdout, and subprocess.stderr streams on the ChildProcess object. This is equivalent to setting the options.stdio equal to ['pipe', 'pipe', 'pipe'].

The docs say that pipe is the default.

Ah - must have done something weird 🤷 . CI failed.

bennn · 2023-08-03T18:58:53Z

🎉 🎉 🎉

robertDurst commented Apr 1, 2023

View reviewed changes

package-lock.json Outdated Show resolved Hide resolved

robertDurst changed the title ~~[WIP] Read Built-in Method~~ [WIP] Read IO Built-in Method Apr 5, 2023

robertDurst force-pushed the rdurst/read_io_builtin branch 2 times, most recently from 90eff05 to 4805807 Compare April 5, 2023 03:42

linux working version

29cc2ba

robertDurst force-pushed the rdurst/read_io_builtin branch from 4805807 to 29cc2ba Compare April 7, 2023 17:28

abstract to other OS's

6a03342

robertDurst added 2 commits June 3, 2023 16:21

WIP, begin using the theOutsideWorld to pass in stdin, stdout fds

77c79cc

hacky first attempt at an io test

427b3f8

bennn marked this pull request as ready for review June 13, 2023 00:24

blerner reviewed Jun 13, 2023

View reviewed changes

robertDurst added 2 commits June 13, 2023 01:45

first attempt at async w/ pause, restart runtime

259517f

remove extra comment

98d60e5

robertDurst commented Jun 13, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

robertDurst commented Jun 13, 2023

View reviewed changes

src/js/trove/iolib.js Outdated Show resolved Hide resolved

blerner reviewed Jun 13, 2023

View reviewed changes

still many TODOs, but narrowing in on more acceptable solution

1f6da56

robertDurst commented Jun 13, 2023

View reviewed changes

src/js/trove/global.js Outdated Show resolved Hide resolved

robertDurst added 6 commits June 13, 2023 16:07

minimize diffs

077746d

fix provides

3df2fb1

fix provides

fc7abea

fix provides

91cdba7

some more nits && an example program demoing guess a number

55c567b

restarter.error?

b3fda53

robertDurst commented Jun 14, 2023

View reviewed changes

robertDurst commented Jun 27, 2023

View reviewed changes

add another test

7ceb982

robertDurst commented Jun 27, 2023

View reviewed changes

robertDurst changed the title ~~[WIP] Read IO Built-in Method~~ Prompt IO Built-in Method Jun 28, 2023

robertDurst added 3 commits June 30, 2023 08:14

add to tests which is called by ci

3384f5b

make tests more robust by separting in/out

3a02a38

small nit to README

67dae10

robertDurst added 4 commits June 30, 2023 11:22

small nitpick before next review

e42ba72

bump node to 16 in ci

bdeb6fe

small nit

9a8e8c8

small nit

ad3fa44

robertDurst commented Jul 3, 2023

View reviewed changes

robertDurst added 3 commits July 3, 2023 09:47

sometimes exit code is null

d9aa748

remove flaky? test

97f9af3

add back flaky? test - bump timeouts

b77a062

robertDurst commented Jul 3, 2023

View reviewed changes

tidy up for another review

f43780d

robertDurst requested a review from blerner July 31, 2023 23:57

blerner reviewed Aug 1, 2023

View reviewed changes

robertDurst added 5 commits July 31, 2023 21:23

speedup tests

70bc0b9

undo breaking of tests

654bbfc

address Ben test optimize feedback

e5c9501

use compile-dir

575db36

need the newline

fb705d5

blerner merged commit f1103c9 into brownplt:horizon Aug 3, 2023

Prompt IO Built-in Method #1688

Prompt IO Built-in Method #1688

Uh oh!

Conversation

robertDurst commented Apr 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Prompt IO Built-in Method

Adherence to Pyret's Philosophy and Core Values

Naming

Type

Example Usage

Note(s) to Reviewer(s)

Uh oh!

Uh oh!

jpolitz commented Apr 5, 2023

Uh oh!

robertDurst commented Apr 7, 2023

Uh oh!

robertDurst commented Apr 10, 2023

Uh oh!

bennn commented Jun 13, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertDurst Jun 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertDurst commented Jun 30, 2023

Uh oh!

robertDurst commented Jun 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertDurst commented Jul 3, 2023

Uh oh!

blerner Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bennn commented Aug 3, 2023

robertDurst commented Apr 1, 2023 •

edited

Loading

robertDurst Jun 14, 2023 •

edited

Loading

robertDurst commented Jun 30, 2023 •

edited

Loading

blerner Aug 1, 2023 •

edited

Loading