On 2013-04-11 17:43, 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!).

Does it throw an Error, that's bad. Code should basically never create and throw an Error or Exception. It should always be a derived type from either of these classes.

-- 
/Jacob Carlborg

On 2013-04-11 20:12, Lars T. Kyllingstad wrote:

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

EnvironmentException, SystemException, ProcessException.

-- 
/Jacob Carlborg

On 2013-04-04 16:22, Jesse Phillips wrote:

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

1. Why is there functionality for handing environment variables in std.process?

2. The class "environment" should be capitalized

-- 
/Jacob Carlborg

On Thursday, 11 April 2013 at 18:32:23 UTC, Jacob Carlborg wrote:
> On 2013-04-04 16:22, Jesse Phillips wrote:
>
>> 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).
>
> 1. Why is there functionality for handing environment variables in std.process?
>
> 2. The class "environment" should be capitalized

Because it is manipulating the environment your process is running in?

True, though I'd then say do what the original did and alias the class to its lowercase. Not that the result is any different though.

On Thursday, 11 April 2013 at 18:32:23 UTC, Jacob Carlborg wrote:
>> 2. The class "environment" should be capitalized

It's implemented using a class, but you use it like an object. I think that is more important.

Lars

On 2013-04-11 22:31, Lars T. Kyllingstad wrote:

> It's implemented using a class, but you use it like an object. I think
> that is more important.

I see. I know there are some people here that are very picky about these things.

-- 
/Jacob Carlborg

On Thursday, 11 April 2013 at 18:20:02 UTC, Jacob Carlborg wrote:
>
> Does it throw an Error, that's bad. Code should basically never create and throw an Error or Exception. It should always be a derived type from either of these classes.

I completely agree.  However, Phobos already throws straight Exceptions all over the place (just grep for "enforce"), so the few places where std.process does so is just in keeping with tradition. ;)

Jokes aside, using Error/Exception until we have decided on a proper exception hierarchy will help reduce the breakage when we do.

Lars

On Friday, 12 April 2013 at 06:36:31 UTC, Jacob Carlborg wrote:
> On 2013-04-11 22:31, Lars T. Kyllingstad wrote:
>
>> It's implemented using a class, but you use it like an object. I think
>> that is more important.
>
> I see. I know there are some people here that are very picky about these things.

Then let me ask you/them this:  Would you prefer it was implemented as a module-level @property function that returns a singleton class instance instead?  In other words, would you have the same API with slightly worse performance and more complicated implementation, *just* for the sake of a strict naming convention?

Note that 'environment' (with that name) was added to std.process years ago.  As Jesse mentioned, the fact that it was a class was just hidden from documentation with an alias.  Unfortunately, this prevented proper documentation of its members, so changed it.

Lars

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

Let's go through the places where an Error or Exception is thrown:

spawnProcess() throws RangeError when args[] is empty, but this is just the normal behaviour of arrays, and with -release/-noboundscheck it just segfaults.  As such, there is little point in specifying this in the documentation.  I'll remove it.  (Honestly, I don't know why I put it in there in the first place.  It may have had something to do with me being thoroughly annoyed over other the lack of exception specifications in Phobos documentation at large.  I used to like enforce(), but now I think it has given D programmers a way too lax attitude towards error handling.)

kill() throws Error if the code/signal is negative.  I suspect the cases where this number comes directly from user input are so few and far between that it is reasonable to expect the programmer to ensure that it is nonnegative.  In principle, on POSIX we don't need the check, because POSIX kill() will return an "invalid signal" error if you try to give it a negative number.  On Windows, however, TerminateProcess() takes an unsigned integer (which is why I added the check in the first place), and I think we should strive to have the same behaviour on all platforms to the extent possible.  I would not be strongly opposed to making this an Exception of some kind, though, if you think there is a good reason to do so.

pipeProcess throws Error on an invalid combination of Redirect flags, and ProcessPipes does the same on an attempt to access a non-redirected stream.  Are we in agreement that these two are always programming errors?

escapeShellCommand() throws Error if the input contains \0, \r or \n.  Here, it is very likely that the arguments are user input, but it would be very strange application code that somehow allowed those three characters to enter the input.  Even so, it may be better to make it an Exception, just to be safe.

Lars

On Friday, 12 April 2013 at 11:31:29 UTC, Lars T. Kyllingstad wrote:
> Let's go through the places where an Error or Exception is thrown:
>
> [...]

I forgot environment, whose methods throw straight Exceptions.  As I've said before, I am not happy about this at all, but it *is* how environment is implemented in the old std.process too.  Thus, leaving it like this until we figure out the whole exception mess is, IMO, better than breaking it twice.

Lars