This is a request for comments.

This is a request for comments.

First attempt at implementing a higher-level API for processes.
Add a startNonInteractive constructor that takes a callback which
is called with the exit code and the complete stdout and stderr
when the process terminates.

Let me know what you think. This obviously needs at least tests.
Not entirely sure what I think about the interface in the first
place.

R=sgjesse@google.com
BUG=1689
TEST=

Committed: https://code.google.com/p/dart/source/detail?r=6499

[Mads Ager (google), 2012-03-26 23:53:26]

Request for comments.

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

Request granted! I don't know if you wanted comments from *me* specifically, but
here they are anyway. ;)

https://chromiumcodereview.appspot.com/9863015/diff/1/runtime/bin/process.dart
File runtime/bin/process.dart (right):

https://chromiumcodereview.appspot.com/9863015/diff/1/runtime/bin/process.dar...
runtime/bin/process.dart:44: Process.startNonInteractive(String executable,
"startNonInteractive" is a mouthful. Maybe just "run"?

https://chromiumcodereview.appspot.com/9863015/diff/1/runtime/bin/process.dar...
runtime/bin/process.dart:46: String workingDirectory,
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.

https://chromiumcodereview.appspot.com/9863015/diff/1/runtime/bin/process.dar...
runtime/bin/process.dart:54: * Throws a [ProcessException] if the process is
non-interactive.
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]

Thanks Bob! :-)

https://chromiumcodereview.appspot.com/9863015/diff/1/runtime/bin/process.dart
File runtime/bin/process.dart (right):

https://chromiumcodereview.appspot.com/9863015/diff/1/runtime/bin/process.dar...
runtime/bin/process.dart:44: Process.startNonInteractive(String executable,
On 2012/03/27 00:20:01, Bob Nystrom wrote:
> "startNonInteractive" is a mouthful. Maybe just "run"?

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

https://chromiumcodereview.appspot.com/9863015/diff/1/runtime/bin/process.dar...
runtime/bin/process.dart:46: String workingDirectory,
On 2012/03/27 00:20:01, Bob Nystrom wrote:
> 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.

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. :-)

https://chromiumcodereview.appspot.com/9863015/diff/1/runtime/bin/process.dar...
runtime/bin/process.dart:54: * Throws a [ProcessException] if the process is
non-interactive.
On 2012/03/27 00:20:01, Bob Nystrom wrote:
> 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.

Sure, I'll change that.

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

https://chromiumcodereview.appspot.com/9863015/diff/1/runtime/bin/process.dart
File runtime/bin/process.dart (right):

https://chromiumcodereview.appspot.com/9863015/diff/1/runtime/bin/process.dar...
runtime/bin/process.dart:46: String workingDirectory,
On 2012/03/27 00:33:25, Mads Ager wrote:
> On 2012/03/27 00:20:01, Bob Nystrom wrote:
> > 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.
> 
> 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. :-)

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?

[zundel, 2012-03-27 00:51:54]

https://chromiumcodereview.appspot.com/9863015/diff/1/runtime/bin/process.dart
File runtime/bin/process.dart (right):

https://chromiumcodereview.appspot.com/9863015/diff/1/runtime/bin/process.dar...
runtime/bin/process.dart:29: * Creates a new process object and starts a process
running the
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?

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

That was a lot of suggestions. After writing them up I currently like adding an
onDone which is exclusive from stdout, stderr and onExit.

However having onDone still needs some work regarding how to specify options on
how the data from stdout and stderr is delivered (encoding, lines, binary).
Thinking about this again maybe for this simple case we should only provide
strings as specified. For lines and binary the advanced interface can be used.
It should be possible to specify encoding though, and maybe we should normalize
line ends to just \n so that String.split can be used safely to get the lines.

https://chromiumcodereview.appspot.com/9863015/diff/1/runtime/bin/process.dart
File runtime/bin/process.dart (right):

https://chromiumcodereview.appspot.com/9863015/diff/1/runtime/bin/process.dar...
runtime/bin/process.dart:44: Process.startNonInteractive(String executable,
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.

https://chromiumcodereview.appspot.com/9863015/diff/1/runtime/bin/process.dar...
runtime/bin/process.dart:46: String workingDirectory,
On 2012/03/27 00:46:00, Mads Ager wrote:
> On 2012/03/27 00:33:25, Mads Ager wrote:
> > On 2012/03/27 00:20:01, Bob Nystrom wrote:
> > > 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.
> > 
> > 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. :-)
> 
> 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?

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.

https://chromiumcodereview.appspot.com/9863015/diff/1/runtime/bin/process.dar...
runtime/bin/process.dart:54: * Throws a [ProcessException] if the process is
non-interactive.
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.

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

https://chromiumcodereview.appspot.com/9863015/diff/1/runtime/bin/process.dart
File runtime/bin/process.dart (right):

https://chromiumcodereview.appspot.com/9863015/diff/1/runtime/bin/process.dar...
runtime/bin/process.dart:29: * Creates a new process object and starts a process
running the
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]

Thanks for the comments guys. It seems clear that this is one of these cases
where everyone will have their own opinion on what is most aesthetically
pleasing. :-) 

I'm currently leaning towards the constructor with an options object and a
required callback argument.

I will play around with this some more. To try actually writing some code with
it I'll see if I can rewrite some of the simpler uses of processes in test.dart
to use the new constructor. Maybe that will change my mind.

http://codereview.chromium.org/9863015/diff/1/runtime/bin/process.dart
File runtime/bin/process.dart (right):

http://codereview.chromium.org/9863015/diff/1/runtime/bin/process.dart#newcode29
runtime/bin/process.dart:29: * Creates a new process object and starts a process
running the
On 2012/03/27 12:33:21, zundel wrote:
> 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.

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.

http://codereview.chromium.org/9863015/diff/1/runtime/bin/process.dart#newcode44
runtime/bin/process.dart:44: Process.startNonInteractive(String executable,
On 2012/03/27 07:03:23, Søren Gjesse wrote:
> 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.

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.

http://codereview.chromium.org/9863015/diff/1/runtime/bin/process.dart#newcode46
runtime/bin/process.dart:46: String workingDirectory,
On 2012/03/27 07:03:23, Søren Gjesse wrote:
> On 2012/03/27 00:46:00, Mads Ager wrote:
> > On 2012/03/27 00:33:25, Mads Ager wrote:
> > > On 2012/03/27 00:20:01, Bob Nystrom wrote:
> > > > 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.
> > > 
> > > 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. :-)
> > 
> > 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?
> 
> 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.

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.

http://codereview.chromium.org/9863015/diff/1/runtime/bin/process.dart#newcode54
runtime/bin/process.dart:54: * Throws a [ProcessException] if the process is
non-interactive.
On 2012/03/27 07:03:23, Søren Gjesse wrote:
> Maybe we should consider one of two:
> 
>  1. make the Process.run a static method and not a constructor

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?

>  2. create a different process class for the simpler running of processes.

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.

>  3. Have both onDone and stderr, stdin and onExit

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...

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

http://codereview.chromium.org/9863015/diff/1/runtime/bin/process.dart
File runtime/bin/process.dart (right):

http://codereview.chromium.org/9863015/diff/1/runtime/bin/process.dart#newcode29
runtime/bin/process.dart:29: * Creates a new process object and starts a process
running the
On 2012/03/27 16:56:32, Mads Ager wrote:
> On 2012/03/27 12:33:21, zundel wrote:
> > 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.
> 
> 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.

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]

http://codereview.chromium.org/9863015/diff/1/runtime/bin/process.dart
File runtime/bin/process.dart (right):

http://codereview.chromium.org/9863015/diff/1/runtime/bin/process.dart#newcode29
runtime/bin/process.dart:29: * Creates a new process object and starts a process
running the
On 2012/03/27 17:48:29, zundel wrote:
> On 2012/03/27 16:56:32, Mads Ager wrote:
> > On 2012/03/27 12:33:21, zundel wrote:
> > > 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.
> > 
> > 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.
> 
> 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.)

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?

[zundel, 2012-03-27 18:32:20]

On 2012/03/27 18:09:43, Mads Ager wrote:
> http://codereview.chromium.org/9863015/diff/1/runtime/bin/process.dart
> File runtime/bin/process.dart (right):
> 
>
http://codereview.chromium.org/9863015/diff/1/runtime/bin/process.dart#newcode29
> runtime/bin/process.dart:29: * Creates a new process object and starts a
process
> running the
> On 2012/03/27 17:48:29, zundel wrote:
> > On 2012/03/27 16:56:32, Mads Ager wrote:
> > > On 2012/03/27 12:33:21, zundel wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > 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.)
> 
> 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?


This is a bit crazy, I had to use a factory method to get it to work with the VM
(not sure why) but it gets the class 'NonInteractiveProcess' into the Process
class listed as a constructor.

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

class Process {
  final bool isInteractive;

  factory NonInteractiveProcess() {
    return new Process._startNonInteractive();
  }

  Process() : isInteractive = true {
  }

  Process._startNonInteractive() : isInteractive = false {
  }

  set onStart (void callback()) {
    callback();
  }

  List<String> get stderr() {
    if (!isInteractive) throw new Exception("Not interactive");
    return <String>["foo", "bar"];
  }

}

main() {
   Process p = new Process();
   NonInteractiveProcess n = new NonInteractiveProcess();

   p.onStart = () => print ("Hello Process!");
   n.onStart = () => print ("Hello Non-interactive Process!");
   print (p.stderr);
   print (n.stderr);
}



Static analysis doesn't like the reference to stderr for noninteractive
processes:

$ dartc t.dart
file:/home/zundel/t.dart:38: "stderr" is not a member of NonInteractiveProcess
    37:    print (p.stderr);
    38:    print (n.stderr);

And neither does runtime:

zundel@santafe:~$ dart t.dart
Hello Process!
Hello Non-interactive Process!
[foo, bar]
Unhandled exception:
Exception: Not interactive
 0. Function: 'Process.get:stderr' url: 'file:///home/zundel/t.dart' line:25
col:25
 1. Function: '::main' url: 'file:///home/zundel/t.dart' line:38 col:19


I tried it with named constructors (new NonInteractiveProcess.start()) and it
works on the VM.

[Mads Ager (google), 2012-04-12 14:19:36]

Søren, could you take another look?

Introduced ProcessOptions argument. Added argument for stdout and stderr
encodings. Added tests.

I think this is as good as it gets in the first version.

[Søren Gjesse, 2012-04-12 15:02:20]

lgtm

https://chromiumcodereview.appspot.com/9863015/diff/10001/runtime/bin/process...
File runtime/bin/process.dart (right):

https://chromiumcodereview.appspot.com/9863015/diff/10001/runtime/bin/process...
runtime/bin/process.dart:9: interface Process default _Process {
As the implementation of start and run use two almost disjunct implementations
of the Process interface then maybe we should consider making two separate
interfaces "Process" and "ProcessRunner". The current approach is also OK so let
us move forward with it as is.

https://chromiumcodereview.appspot.com/9863015/diff/10001/runtime/bin/process...
runtime/bin/process.dart:111: * [ProcessOptions]q represents the options that
can be supplied when
q?

https://chromiumcodereview.appspot.com/9863015/diff/10001/runtime/bin/process...
runtime/bin/process.dart:132: Encoding stdoutEncoding;
The encodings are gnored when passing this to Process.start. Should we just
ignore that or throw an exception if one of then are not null in that case?

https://chromiumcodereview.appspot.com/9863015/diff/10001/runtime/bin/process...
runtime/bin/process.dart:143: Encoding stderrEncoding;
Should there be a string or bytearray that will be piped in through stdin when a
process is run?

https://chromiumcodereview.appspot.com/9863015/diff/10001/runtime/bin/process...
File runtime/bin/process_impl.dart (right):

https://chromiumcodereview.appspot.com/9863015/diff/10001/runtime/bin/process...
runtime/bin/process_impl.dart:318: if (stdoutEncoding is !Encoding) {
Maybe we should get rid of this type check and move it to the StringInputStream
constructor instead.

https://chromiumcodereview.appspot.com/9863015/diff/10001/runtime/bin/process...
runtime/bin/process_impl.dart:325: if (stderrEncoding is !Encoding) {
Ditto.

[Mads Ager (google), 2012-04-13 08:49:39]

https://chromiumcodereview.appspot.com/9863015/diff/10001/runtime/bin/process...
File runtime/bin/process.dart (right):

https://chromiumcodereview.appspot.com/9863015/diff/10001/runtime/bin/process...
runtime/bin/process.dart:111: * [ProcessOptions]q represents the options that
can be supplied when
On 2012/04/12 15:02:21, Søren Gjesse wrote:
> q?

Done.

https://chromiumcodereview.appspot.com/9863015/diff/10001/runtime/bin/process...
runtime/bin/process.dart:132: Encoding stdoutEncoding;
On 2012/04/12 15:02:21, Søren Gjesse wrote:
> The encodings are gnored when passing this to Process.start. Should we just
> ignore that or throw an exception if one of then are not null in that case?

I went for ignoring. I don't think it matters much as long as we document the
behavior. I would be fine with changing this if someone finds a compelling
reason to change it.

https://chromiumcodereview.appspot.com/9863015/diff/10001/runtime/bin/process...
runtime/bin/process.dart:143: Encoding stderrEncoding;
On 2012/04/12 15:02:21, Søren Gjesse wrote:
> Should there be a string or bytearray that will be piped in through stdin when
a
> process is run?

Yes, there probably should. I'll add that in a separate change. Similarly, there
should be a way to set environment variables for the spawned process.

https://chromiumcodereview.appspot.com/9863015/diff/10001/runtime/bin/process...
File runtime/bin/process_impl.dart (right):

https://chromiumcodereview.appspot.com/9863015/diff/10001/runtime/bin/process...
runtime/bin/process_impl.dart:318: if (stdoutEncoding is !Encoding) {
On 2012/04/12 15:02:21, Søren Gjesse wrote:
> Maybe we should get rid of this type check and move it to the
StringInputStream
> constructor instead.

It will throw an exception through the StringInputStream constructor in any case
if it is not one of the known encodings.

However, I like the more specific error message you get by having the check
here.