On 2013-04-04 16:22, Jesse Phillips wrote:
> Hello,
>
> I have decided to take on Review Manager for std.process and hopefully
> will keep it up and get through the review queue. This is slightly
> ignoring a "first come, first serve" approach, but that is mostly
> because I didn't want to look into it. (So don't be too upset).
>
> std.process has been under review for some time having discussion on the
> pull request and this thread:
> http://forum.dlang.org/thread/stxxtfwfrwllkcpunhue@forum.dlang.org
>
> I would like to propose the Formal review is one week (and one week
> voting). If there is objection speak up.

Why does all functions taking stdin/stdout/stderr append an underscore to these parameter names?

-- 
/Jacob Carlborg

Am Sun, 07 Apr 2013 20:52:11 +0200
schrieb "Lars T. Kyllingstad" <public@kyllingen.net>:

> On Friday, 5 April 2013 at 05:54:39 UTC, Lars T. Kyllingstad wrote:
> > The following is a message posted by Johannes Pfau to the other (pre-)review thread which didn't seem to get noticed, along with an edited version of my reply.
> >
> > On Sunday, 31 March 2013 at 13:14:52 UTC, Johannes Pfau wrote:
> >>
> >> Reposted from github:
> >> I think it would be nice if the high level functions would
> >> also allow
> >> using custom environment variables. So the execute and
> >> executeShell
> >> functions should have overloads which accept a string[string]
> >> with
> >> environment variables (and probably accept a Config as well).
> >>
> >> The execute and executeShell functions would be trivial to
> >> implement
> >> though if pipeProcess / pipeShell had an overload with
> >> environment
> >> and Config parameters. So it's probably more important that
> >> pipeProcess / pipeShell get these overloads.
> 
> I have implemented this now.  I think this was a great improvement.  pipeProcess(/-Shell) and execute(Shell) now wield more of the power of spawnProcess(/-Shell) without complicating their usage.
> 
> Please comment.
> 
> Lars

Thanks!
Looks good to me :-)

On Monday, 8 April 2013 at 06:48:43 UTC, Jacob Carlborg wrote:
> Why does all functions taking stdin/stdout/stderr append an underscore to these parameter names?

Because there are symbols with those names defined in both std.stdio and core.stdc.stdio, and I got naming conflicts all over the place.  This was a while ago, though, and the compiler may have seen improvements in this area since then.  I'll try it out later.

Lars

On 2013-04-08 11:17, Lars T. Kyllingstad wrote:

> Because there are symbols with those names defined in both std.stdio and
> core.stdc.stdio, and I got naming conflicts all over the place.  This
> was a while ago, though, and the compiler may have seen improvements in
> this area since then.  I'll try it out later.

I would say that something's wrong if one needs to come up with special naming schemes for function arguments.

-- 
/Jacob Carlborg

On Monday, 8 April 2013 at 11:17:27 UTC, Jacob Carlborg wrote:
> On 2013-04-08 11:17, Lars T. Kyllingstad wrote:
>
>> Because there are symbols with those names defined in both std.stdio and
>> core.stdc.stdio, and I got naming conflicts all over the place.  This
>> was a while ago, though, and the compiler may have seen improvements in
>> this area since then.  I'll try it out later.
>
> I would say that something's wrong if one needs to come up with special naming schemes for function arguments.

You sound surprised that there is something wrong in the world of software development. :)

I've changed the parameter names now.  The auto-tester seems to approve, so I guess the compiler bugs have been fixed.  Thanks for pointing this out!

Lars

On 2013-04-08 19:32, Lars T. Kyllingstad wrote:

> You sound surprised that there is something wrong in the world of
> software development. :)

Hehe, oh no, I would be surprised if it wasn't :)

> I've changed the parameter names now.  The auto-tester seems to approve,
> so I guess the compiler bugs have been fixed.  Thanks for pointing this
> out!

Great.

-- 
/Jacob Carlborg

A couple minor comments:

1. I have two issues with Error being used.  One is that we should have a specific type that is thrown, not raw Error type.  Second is that I think in situations where the error is due to an incorrect parameter, it should be an exception not an error (and not a straight Exception either!).
2. For the documentation of inheritFDs, it says that "On Windows, this option has no effect."  While this is true, it may give the impression that we don't set the bInheritHandles flag, which we do.  I think there should be a note on that somewhere.

Otherwise, the API and functionality looks good!

-Steve

The doc build linked in the first post says that the execute* return type might change to std.typecons.Tuple!(int,"status",bool,"output") – I suppose this should be »…, string, "output"«.

Unfortunately, I probably won't manage to do an actual review before the deadline, but I didn't notice any obvious major issues.

David

On Thursday, 11 April 2013 at 15:43:18 UTC, Steven Schveighoffer wrote:
> A couple minor comments:
>
> 1. I have two issues with Error being used.  One is that we should have a specific type that is thrown, not raw Error type.
>  Second is that I think in situations where the error is due to an incorrect parameter, it should be an exception not an error (and not a straight Exception either!).

I agree for the most part.  The 'environment' functions do throw straight Exceptions, and I'm not happy about that.  I just don't like the idea of adding even more module-specific Exceptions and Errors, when what we really should do is decide on a systematic organisation for the entire library.

Then again, maybe such a reorganisation will cause enough breakage that one more module makes little difference?  Any good name suggestions for the errors and exceptions in question?

> 2. For the documentation of inheritFDs, it says that "On Windows, this option has no effect."  While this is true, it may give the impression that we don't set the bInheritHandles flag, which we do.  I think there should be a note on that somewhere.

I'll add one.

Lars

On Thursday, 11 April 2013 at 17:56:17 UTC, David Nadlinger wrote:
> The doc build linked in the first post says that the execute* return type might change to std.typecons.Tuple!(int,"status",bool,"output") – I suppose this should be »…, string, "output"«.

That's right.  I'll fix it.

Lars