This is a request for comments.

runtime/bin/process.dart

Index: runtime/bin/process.dart
diff --git a/runtime/bin/process.dart b/runtime/bin/process.dart
index 181f6c6e7f6f0bbfa1e0a43a071bc7c8d7f908e8..5f3f53ecc757df1ca8faec050f451219e0749850 100644
--- a/runtime/bin/process.dart
+++ b/runtime/bin/process.dart
@@ -26,17 +26,46 @@ interface Process default _Process {
                 [String workingDirectory]);
 
   /**
+   * Creates a new process object and starts a process running the

[zundel, 2012/03/27 00:51:55]

Why didn't you didn't create a subclass of Process?  I know subclassing can
start to limit your options, but   All the new methods don't work with regular
processes and will get in the way of those using regular (interactive)
processes.  Would it make it easier for the dart2js compiler to tree-shake out? 

[zundel, 2012/03/27 12:33:21]

I see now the relationship is inverted to what I originally thought - I thought
stderr, stdout were being added, but these are just new comments.

Just to expound a little further on what I was thinking: Many developers using
an IDE will use the code completion feature in an editor to explore an API.   I
am suggesting it can be confusing when code completion calls up many methods
that are invalid on the context of the call - there is no easy way of statically
knowing that the instance was created with Process.startNonInteractive() -
splitting into different classes or interfaces makes this apparent to ide users.

[Mads Ager (google), 2012/03/27 16:56:32]

I see your point Eric. However, I think that having a separate class for this
leads to another discoverability problem. Now you need to find the class to use.
At least if it is just Process, you know where to look and can read
documentation.

[zundel, 2012/03/27 17:48:29]

This is more work for you, so I can understand why you might not want to do it,
but potentially, it could be discoverable by using an interface and the default
clause

interface NonInteractiveProcess default Process {
  NonInteractiveProcess();
  void set onStart(void callback());
  void set onError(void callback());
  void kill();
  void close();
}

class Process {  // don't use implements NonInteractiveProcess for constructor
naming reasons
  NonInteractiveProcess() {
    ... constructor for a NonInteractiveProcess() ...
  }
  ... Other process methods ...
}

It isn't clear from reading the docs on whether this works with named
constructors which is the convention you've established (and I don't see any
tests that exercise it.)

[Mads Ager (google), 2012/03/27 18:09:43]

I'm not sure I follow your suggestion. You still have to discover the
NonInteractiveProcess type and use it in your code to have the scaled down
interface? I don't think the code above is legal? Could you provide a scaled
down working example that illustrates the advantages of this approach?

+   * [executable] with the specified [arguments]. When the process has
+   * been successfully started [onStart] is called. If the process
+   * fails to start [onError] is called.
+   *
+   * [workingDirectory] specify where the process is run from. Note
+   * that the change of directory occurs before executing the process
+   * on some platforms, which may have impact when using relative
+   * paths for [executable] and [arguments].
+   *
+   * No communication via [stdin], [stdout] or [stderr] can take place
+   * with a non-interactive process. Instead, the process is run to
+   * completion at which point the exit code and stdout and stderr are
+   * supplied to the [callback] parameter.
+   */
+  Process.startNonInteractive(String executable,

[Bob Nystrom, 2012/03/27 00:20:01]

"startNonInteractive" is a mouthful. Maybe just "run"?

[Mads Ager (google), 2012/03/27 00:33:25]

Yes, will do. The comment should state in the first line that the process is run
non-interactively to completion.

[Søren Gjesse, 2012/03/27 07:03:23]

The callback provides stdout and stderr as strings. However in some situations
it could be convenient to have these as bytes or as lines with the line breaking
done by StringInputStream already applied.

Also to get strings we should have a way of specifying the encoding used for
both streams.

[Mads Ager (google), 2012/03/27 16:56:32]

Adding support for specifying an encoding of the output seems like a good thing
to do. For more advanced use cases I think it would be fine to require the
low-level interface to be used.

+                              List<String> arguments,
+                              String workingDirectory,

[Bob Nystrom, 2012/03/27 00:20:01]

Seeing this repeated list of arguments between this and start is a code smell to
me. Consider pulling those into a separate objects? I expect that this set of
arguments will grow over time and positional arguments don't do that well.
Objects do.

See, for example, ProcessStart in C#.

Also, workingDirectory should really be optional, but that doesn't play nice
with the callback being the last argument and positional. I see this wart show
up frequently in our APIs (see, for example, registering event listeners with
the capture: argument) and I think we need a better solution. This really makes
me wish we had Smalltalk/Ruby-style blocks.

[Mads Ager (google), 2012/03/27 00:33:25]

I completely agree that we need a better way of handling this. That was the
reason for sending this out now as a request for comments. I wanted to hear if
others felt the same way. They do. :-)

[Mads Ager (google), 2012/03/27 00:46:00]

I would like to keep the simple case very easy. So maybe a compromise where
[executable] and [arguments] are passed directly and everything else (working
directory, timeout, whatever...) is wrapped in a ProcessOptions object that you
can pass in. For the simple case you can use 'null'. That way we still keep the
simplicity of the simple common case:

new Process.start('ls', '[-l]');
new Process.run('ls', '[-l]', null, (code, out, err) {
  ...
});

How does that sound?

[Søren Gjesse, 2012/03/27 07:03:23]

I am not sure about the ProcessOptions argument - it will make the simple things
simple, but make specifying something in addition to the simple case more
tedious. How about just using optional arguments, and make the callback optional
as well?

new Process.run('ls',
                '[-l]',
                 workindDirectory: "/",
                 onDone: (code, out, err) {
  ...
});

instead of

new Process.run('ls',
                '[-l]',
                 new ProcessOptions(workingDirectory: "/"),
                 (code, out, err) {

Specify other options

new Process.run('ls',
                '[-l]',
                 workindDirectory: "/",
                 stdoutEncoding: Encoding.UTF_8,
                 stdoutLineBreaking: true,
                 stderrEncoding: Encoding.NONE,
                 onDone: (code, out, err) {
  assert(out is List<String>);
  assert(err is List<int>);
  ...
});

vs.

new Process.run('ls',
                '[-l]',
                 new ProcessOptions(workindDirectory: "/",
                                    stdoutEncoding: Encoding.UTF_8,
                                    stdoutLineBreaking: true,
                                    stderrEncoding: Encoding.NONE),
                 (code, out, err) {
  assert(out is List<String>);
  assert(err is List<int>);
  ...
});

except for when you want to reuse the process options I am not sure it buys
much.

[Mads Ager (google), 2012/03/27 16:56:32]

I think what it buys is that you get away from the argument list. When you cram
everything in there as in the examples above it doesn't give you anything.
However, if you split it out:

var options = new ProcessOptions();
options.workingDirectory = '/';
options.stdoutEncoding = Encoding.UTF_8;
var p = new Process.run('ls', ['-l'], options, (code, out, err) {
  ...
});

I think it reads much nicer than the long list of arguments to the constructor.

+                              void callback(int exitCode,
+                                            String stdout,
+                                            String stderr));
+
+  /**
    * Returns an input stream of the process stdout.
+   *
+   * Throws a [ProcessException] if the process is non-interactive.

[Bob Nystrom, 2012/03/27 00:20:01]

I don't think this should be a ProcessException. Accessing this on a
non-interactive process is a programmatic error, and I think (strongly) that
those should be a separate exception class from ProcessException, which is used
for other runtime errors.

Java has IllegalStateException and C# has InvalidOperatorException for things
like this. Dart should too.

[Mads Ager (google), 2012/03/27 00:33:25]

Sure, I'll change that.

[Søren Gjesse, 2012/03/27 07:03:23]

Maybe we should consider one of two:

 1. make the Process.run a static method and not a constructor
 2. create a different process class for the simpler running of processes.
 3. Have both onDone and stderr, stdin and onExit

1) Then we would have something like this:

  Process.run('ls'
              ['-l']
              onStart: (...) {
      ...
    },
              onDone: (code, out, err) {
      ...
    },
              onError: (e) {
      ...
    });

maybe not the most elegant construct, but one can use local functions for
onStart, onDone and onError. Also this makes all three callback functions being
speficied in the same way.

2) Would look like:

  SimpleProcess p = new SimpleProcess.run('ls', ['-l']);
  p.onStart = (...) {
    ...
  };
  p.onDone = (code, out, err) {
    ...
  };
  p.onError = (e) {
      ...
  };

"SimpleProcess" is probably not the right name.

3) If using onDone then stdout, stderr and onExit cannot be used and vise-versa.

  Process p = new Process.start('ls', ['-l']);
  p.onDone = (code, out, err) {
    ...
  };
  p.stdout;  // Throws an exception.
  
and

  Process p = new Process.start('ls', ['-l']);
  p.stdout.onData = () => null;  
  p.onDone = (code, out, err) => null; // Throws an exception.
  
in both cases onStart and onError can be used.

[Mads Ager (google), 2012/03/27 16:56:32]

That would be a possibility. However, since you are really constructing a
process object that you can kill here I think we should stick with constructors.
Having this not be a constructor we at least have to think about the consistency
of the APIs. If we go that route, we should reconsider the other constructor in
the system and see if it should really be a static. I'm not sure what the
criterion for something being a constructor or a static should be?
That could be done. I can't come up with a good name though. I'm afraid that
discoverability would suffer. Now you have to find some other class instead of
just invoking a different constructor.
This was what I was trying to avoid. Since these processes are really
'single-shot' I wanted to pass in the callback directly as we do with other
single-shot things to get the immediacy and just have a restricted interface.
The thing that sticks out a little here is that you have a onStart callback in
addition to onError...

    */
   InputStream get stdout();
 
   /**
    * Returns an input stream of the process stderr.
+   *
+   * Throws a [ProcessException] if the process is non-interactive.
    */
   InputStream get stderr();
 
   /**
    * Returns an output stream to the process stdin.
+   *
+   * Throws a [ProcessException] if the process is non-interactive.
    */
   OutputStream get stdin();
 
@@ -47,7 +76,10 @@ interface Process default _Process {
   void set onStart(void callback());
 
   /**
-   * Sets an exit handler which gets invoked when the process terminates.
+   * Sets an exit handler which gets invoked when the process
+   * terminates.
+   *
+   * Throws a [ProcessException] if the process is non-interactive.
    */
   void set onExit(void callback(int exitCode));