Implementation of raw_input() #233

corona10 · 2017-02-03T16:40:33Z

I implemented a simplified version of raw_input().

Please review on focusing these.
First, It might be not 100% compatible with CPython and pypy's raw_input().
and also need more implementation to handle error cases.

Second, I need some guide for writing unit test codes to check it is well working for raw_input().
I don't know how to test user's input from keyboard.

Third, prompt printing is slower when running with go run *.go so if you have solutions then please give me a feedback.

Related PR= #247

trotterdylan

As far as unit tests go, it should be pretty straightforward to stub out os.Stdin with a different File object and then read from that object. See the Print() tests to understand what I mean.

Re: performance: My guess is that go run is compiling a bunch of stuff before actually running the program. go run and go test don't do much caching of compiled artifacts between runs so that's probably what's causing the delay.

trotterdylan · 2017-02-04T22:22:00Z

runtime/builtin_types.go

+	}
+
+	fin := os.Stdin
+	fout := os.Stdout


Rather than writing to Stdout directly, you could use the Print() function: Print(f, args, false)

trotterdylan · 2017-02-04T22:43:03Z

runtime/builtin_types.go

+		prompt, _ := ToStr(f, args[0])
+		fmt.Fprint(fout, prompt.Value())
+	}
+	s, _, _ := in.ReadLine()


ReadLine() isn't ideal here because it can return with less than a full line. I think the right thing to do is probably use the grumpy.File object representing sys.stdin and call readLine() on it. But there's no clear way to get the sys module from here.

One possible solution that would be to create Stdin, Stdout and Stderr variables in the grumpy package that wrap os.Stdin/out/err with a grumpy.File object, and then have the sys module expose those instead of creating new files. The grumpy.Print() function could then use the new Stdout object. This isn't perfect but I think it is an improvement over what we have today.

trotterdylan · 2017-02-04T22:44:28Z

runtime/builtin_types.go

+	s, _, _ := in.ReadLine()
+
+	// Todo: Should implement more cases of error.
+	pySsizeTmax := ((^uint(0)) - 1) >> 1


I think that if len(s) were ever > than this value that Go would panic so don't bother checking this.

trotterdylan · 2017-02-04T22:45:29Z

runtime/builtin_types.go

+		return nil, f.RaiseType(OverflowErrorType, msg)
+	}
+
+	return NewStr(string(s[0:size])).ToObject(), nil


How is this different than NewStr(s)?

corona10 · 2017-02-05T00:00:26Z

@trotterdylan Thank you for great and awesome review. I will try to fix it.

corona10 · 2017-02-06T03:59:28Z

@trotterdylan Updated! I fixed codes as you commented. Actually, about using grumpy.File object is not 100% same as you intended. I found the simpler way.
And I add unit test code, but it should be improvement later time. I wish other guys could improve this unit test to check user's input and function's output. Unfortunately, In now time, this is out of my ability.

PTAL

trotterdylan · 2017-02-06T21:06:26Z

runtime/builtin_types.go

+		pyPrint(f, args, "", "", stdout)
+	}
+
+	line, _ := stdin.readLine(MaxInt)


I think I was wrong about implementing it this way :(

The trouble is that the C implementation uses fgets on the underlying C file pointer. fgets reads until it reaches \n but readLine will stop when it reaches \r and will convert that to a \n.

Probably the simplest thing to do here is to call stdin.reader.ReadString("\n") instead. This would mean you'd have to handle the EOF error case, however.

trotterdylan · 2017-02-06T21:07:22Z

runtime/builtin_types.go

+		pyPrint(f, args, "", "", stdout)
+	}
+
+	line, _ := stdin.readLine(MaxInt)


You must handle the error case. It's pretty simple to do:

if err != nil { return nil, f.RaiseType(IOErrorType, err.Error()) }

trotterdylan · 2017-02-06T21:14:43Z

runtime/builtin_types.go

+	}
+
+	stdout := os.Stdout
+	stdin := NewFileFromFD(os.Stdin.Fd())


The trouble with creating a new File wrapping os.Stdin is that it will have a different buffered reader than the one held by sys.stdin. This is especially problematic because this File object is cleaned up after builtinRawInput() returns, meaning that whatever buffered data it held just disappears.

This goes back to my suggestion of having grumpy.Stdin/out/err variables that get used by this function and having sys.stdin/out/err be aliases for those variables:

# sys.py from __go__.grumpy import Stdin as stdin, Stdout as stdout, Stderr as stderr

This way any buffering done for one call will be used by subsequent calls.

trotterdylan · 2017-02-06T21:16:49Z

runtime/builtin_types_test.go

@@ -422,3 +422,24 @@ func TestBuiltinSetAttr(t *testing.T) {
 		}
 	}
 }
+
+// TODO: Improvment unit tests codes into catch user's input and output.


I'd want to see some additional tests that exercise the whole builtinRawInput() function before I approve this PR. It's important that new code is fully tested because the probability that somebody retroactively adds tests is low.

corona10 · 2017-02-07T04:07:12Z

@trotterdylan A lot of stuff!! I will do it as your comment. Thanks

corona10 · 2017-02-09T22:47:35Z

@trotterdylan Updated!! Please take a look.

trotterdylan · 2017-02-11T16:48:43Z

runtime/builtin_types.go

+		return nil, f.RaiseType(TypeErrorType, msg)
+	}
+
+	osStdout := os.Stdout


Use the new grumpy.Stdout variable for output. This will require modifying pyPrint(), Print(), etc. to take a *grumpy.File instead of *os.File. Might be better to add Stdin/out/err and change the print behavior in a separate PR and once that's merged, continue with this one.

trotterdylan · 2017-02-11T16:51:58Z

runtime/builtin_types_test.go

+		// Create a fake Stdin for input test.
+		stdinFile, _ := ioutil.TempFile(os.TempDir(), "prefix")
+		stdinFile.Write([]byte("hello grumpy\n"))
+		stdinFile.Close()


I don't think you need to reopen stdinFile. You should be able to read what you've written. You may need to Sync() (and possibly Seek() to the beginning, but I don't think so).

trotterdylan · 2017-02-11T16:52:08Z

runtime/builtin_types_test.go

+func TestRawInput(t *testing.T) {
+	fun := newBuiltinFunction("TestRawInput", func(f *Frame, args Args, _ KWArgs) (*Object, *BaseException) {
+		// Create a fake Stdin for input test.
+		stdinFile, _ := ioutil.TempFile(os.TempDir(), "prefix")


You need to check errors for these io operations.

trotterdylan · 2017-02-11T16:52:53Z

runtime/builtin_types_test.go

+func TestRawInput(t *testing.T) {
+	fun := newBuiltinFunction("TestRawInput", func(f *Frame, args Args, _ KWArgs) (*Object, *BaseException) {
+		// Create a fake Stdin for input test.
+		stdinFile, _ := ioutil.TempFile(os.TempDir(), "prefix")


Not sure if it'd be easier, but you could also use a Pipe() instead of a temp file for stdin, similar to what you've done for stdout. Something like this:

stdinFile, w := os.Pipe() go func() { w.Write([]byte("hello grumpy\n")) w.Close() }()

You need to do the write asynchronously this way in case the Write() call blocks waiting for a Read() on the other end of the pipe.

Cool~! I will try it!!

trotterdylan · 2017-02-11T16:56:37Z

runtime/builtin_types_test.go

+			os.Stdout = oldStdout
+			Stdin = oldStdin
+			stdinFile.Close()
+			os.Remove(stdinFile.Name())


Move defer os.Remove(stdinFile.Name()) up to where you create the file so that an early return won't leave the tempfile lying around.

trotterdylan · 2017-02-11T16:57:16Z

runtime/builtin_types_test.go

+		if raised != nil {
+			return nil, raised
+		}
+		result := newTestTuple(input, output.String()).ToObject()


return newTestTuple(input, output.String()).ToObject(), nil

corona10 · 2017-02-13T20:36:36Z

@trotterdylan Updated. PTAL

trotterdylan · 2017-02-14T06:02:33Z

runtime/builtin_types.go

+	}
+
+	if len(args) == 1 {
+		pyPrint(f, args, "", "", Stdout)


Check the resulting exception

trotterdylan · 2017-02-14T06:03:39Z

runtime/builtin_types_test.go

+		}
+
+		go func() {
+			defer w.Close()


Don't bother defering, just Close after the Write

trotterdylan · 2017-02-14T06:03:59Z

runtime/builtin_types_test.go

+		go func() {
+			defer w.Close()
+			w.Write([]byte("hello grumpy\n"))
+			stdinFile.Sync()


It's w that needs to Sync, but closing it will do the same thing.

trotterdylan · 2017-02-14T06:06:06Z

runtime/builtin_types_test.go

+		}()
+
+		// Create a fake Stdout for output test.
+		r, stdoutFile, err := os.Pipe()


Any way we can use captureStdout for this part?

@trotterdylan I want to do by the same way you mentioned. But in my opinion, until we refactoring captureStdout, I think not possible right now.

If we could change this part for general functions. It might be possible.

What specifically needs to be generalized? Any inputs or outputs can made part of the closure for the passed function and read or written inside the function call.

@trotterdylan In this case, builtinRawInput() will return *Object and *BaseException, but fn in captureStdout want to return only *BaseException. It will works for any function I will pass?

I'm probably oversimplifying, but would something like this work?

var input *Object raised := captureStdout(func() *BaseException { input = builtinRawInput(...) }) if raised != nil { ... } // validate input

@trotterdylan Awesome! You are right!! Thank you for guiding me. 👍

trotterdylan · 2017-02-14T06:07:11Z

runtime/builtin_types_test.go

+		var raised *BaseException
+
+		go func() {
+			defer close(done)


No need to defer either of these, just call them normally below

trotterdylan · 2017-02-14T06:08:19Z

runtime/builtin_types_test.go

+		}()
+		done := make(chan struct{})
+		var input *Object
+		var output bytes.Buffer


Declare this where it's used down below when passed to Copy

corona10 · 2017-02-14T07:53:20Z

@trotterdylan Updated.

p.s Can I get good materials to know when to use defer? I'm just using it as finally statement.

trotterdylan · 2017-02-14T20:47:17Z

@corona10 re: defer: It can be used like finally. But, like finally, it's only really useful if there are multiple possible paths for exiting the function or an exception (panic in Go) is raised. In this case, a panic is a fatal error so it doesn't really matter if we close things. The only time it's really important to run some code in the case of panic is if the panic may be handled somewhere (for the most part in the Grumpy code, we assume panic is fatal) or if you need to release some resource even in the event of a panic. Sometimes mutex falls into this category because it can otherwise cause a deadlock if not released. It's also good to guarantee that temp files are cleaned up. But otherwise, the possibility of panic is not usually a good reason to defer.

corona10 · 2017-02-16T01:48:30Z

@trotterdylan Thank you for review and great guide! I updated it and please take a look. Thanks to you, I learned about how Go works.

trotterdylan

This is looking great. The main thing now is to add a few more test cases.

trotterdylan · 2017-02-16T17:21:22Z

runtime/builtin_types_test.go

+
+		go func() {
+			w.Write([]byte("hello grumpy\n"))
+			w.Sync()


I think you want to close w here since that's the end of stdin.

trotterdylan · 2017-02-16T17:28:26Z

runtime/builtin_types_test.go

+	}).ToObject()
+	cases := []invokeTestCase{
+		{args: wrapArgs(), want: newTestTuple("hello grumpy", "").ToObject()},
+		{args: wrapArgs("abc"), want: newTestTuple("hello grumpy", "abc").ToObject()},


I think there's a few more things that need testing:

Multiline prompt parameter: make sure all string lines are printed

Multiple lines on stdin: only the first line should be returned by the raw_input call

An empty stdin should raise EOFError:

$ python -c 'raw_input()' < /dev/null Traceback (most recent call last): File "<string>", line 1, in <module> EOFError: EOF when reading a line

The last two test cases can be supported by having fun take a parameter that is the string to put on stdin.

1) Add more testcases. 2) Close w since that's the end of stdin. 3) Add EOFErrorType.

corona10 · 2017-02-17T22:18:48Z

@trotterdylan Updated!! PTAL

trotterdylan · 2017-02-18T17:07:51Z

runtime/builtin_types_test.go

@@ -425,6 +425,69 @@ func TestBuiltinSetAttr(t *testing.T) {
 	}
 }

+func TestRawInput(t *testing.T) {
+	inputTest := func(s string, cases []invokeTestCase) {


This organization seems like a reasonable way to support an additional parameter, but it's a little unorthodox. The way I've usually done it is something like this:

fun := wrapFuncForTest(func(f *Frame, s string, args ...*Object) (*Object, *BaseException) { ... go func() { w.Write([]byte(s)) w.Close() }() ... in, raised := builtinRawInput(f, args, nil) ... }) ... {args: wrapArgs("hello\n", "show"), want: newTestTuple("hello", "show").ToObject()},

trotterdylan

This is great! Thanks for all the iterations. I'm really happy with the end result. Merging.

corona10 · 2017-02-19T00:44:51Z

@trotterdylan Thank you for reviewing long days :-)

pombredanne · 2017-03-02T10:42:40Z

@corona10 very nice :)

corona10 added 2 commits February 3, 2017 14:47

Simplified implementation of raw_input()

eabe6eb

fix.

bdfc7c6

trotterdylan suggested changes Feb 4, 2017

View reviewed changes

corona10 changed the title ~~Implementation of raw_input().~~ [WIP]Implementation of raw_input(). Feb 5, 2017

corona10 added 2 commits February 4, 2017 23:51

Fix

d9ff589

Add unittest

075532a

corona10 changed the title ~~[WIP]Implementation of raw_input().~~ Implementation of raw_input() Feb 6, 2017

trotterdylan suggested changes Feb 6, 2017

View reviewed changes

corona10 changed the title ~~Implementation of raw_input()~~ [WIP]Implementation of raw_input() Feb 7, 2017

trotterdylan's comment

a096994

corona10 changed the title ~~[WIP]Implementation of raw_input()~~ Implementation of raw_input() Feb 9, 2017

More test.

7d1f71a

trotterdylan suggested changes Feb 11, 2017

View reviewed changes

corona10 changed the title ~~Implementation of raw_input()~~ [WIP] Implementation of raw_input() Feb 12, 2017

wip

46ed7a3

corona10 mentioned this pull request Feb 13, 2017

Switch pyPrint() into based on *grumpy.File #247

Merged

corona10 added 4 commits February 13, 2017 13:30

Merge branch 'master' into raw_input

f385580

Change from os.stdout to Stdout.

6a2d03c

fix

46f6e5f

Fix unittest.

6b2dab8

corona10 changed the title ~~[WIP] Implementation of raw_input()~~ Implementation of raw_input() Feb 13, 2017

trotterdylan suggested changes Feb 14, 2017

View reviewed changes

trotterdylan's comment.

1a8f9da

Change unittest to use captureStdout.

347b496

trotterdylan suggested changes Feb 16, 2017

View reviewed changes

trotterdylan's comment.

8e2d7fd

1) Add more testcases. 2) Close w since that's the end of stdin. 3) Add EOFErrorType.

trotterdylan reviewed Feb 18, 2017

View reviewed changes

Use wrapFuncForTest.

9abd463

trotterdylan approved these changes Feb 18, 2017

View reviewed changes

trotterdylan merged commit 498b88e into google:master Feb 18, 2017

corona10 deleted the raw_input branch February 19, 2017 02:45

This was referenced Aug 21, 2018

[CLOSED] Implementation of raw_input() grumpyhome/grumpy#33

Closed

Migrate google/grumpy issues to grumpyhome/grumpy grumpyhome/grumpy#8

Closed

Implementation of raw_input() #233

Implementation of raw_input() #233

Conversation

corona10 commented Feb 3, 2017 • edited Loading

trotterdylan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corona10 commented Feb 5, 2017

corona10 commented Feb 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corona10 commented Feb 7, 2017

corona10 commented Feb 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corona10 commented Feb 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corona10 Feb 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corona10 commented Feb 14, 2017

trotterdylan commented Feb 14, 2017

corona10 commented Feb 16, 2017

trotterdylan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corona10 commented Feb 17, 2017

Choose a reason for hiding this comment

trotterdylan left a comment

Choose a reason for hiding this comment

corona10 commented Feb 19, 2017

pombredanne commented Mar 2, 2017

corona10 commented Feb 3, 2017 •

edited

Loading

corona10 commented Feb 6, 2017 •

edited

Loading

corona10 Feb 14, 2017 •

edited

Loading