Okay, here's the new code and its documentation: http://is.gd/ktURa

I followed Andrei's suggestion and merged most of the functions into a highly flexible assertPred. I also renamed the functions as suggested and attempted to fully document everything with fully functional examples instead of examples using types or functions which don't actually exist.

So, now there's just assertThrown, assertNotThrown, collectExceptionMsg, and assertPred (though there are eight different overloads of assertPred). So, review away.

From the sounds of it, if this code gets voted in, it'll be going into std.exception.

- Jonathan M Davis

On 10/01/11 23:29, Jonathan M Davis wrote:
>  From the sounds of it, if this code gets voted in, it'll be going into
> std.exception.
>
> - Jonathan M Davis

May it be asked by what authority you can say that (ie. as said above)?

On Monday 10 January 2011 05:40:39 Justin Johansson wrote:
> On 10/01/11 23:29, Jonathan M Davis wrote:
> >  From the sounds of it, if this code gets voted in, it'll be going into
> > 
> > std.exception.
> > 
> > - Jonathan M Davis
> 
> May it be asked by what authority you can say that (ie. as said above)?

Andrei's posts in this thread. Assuming that the code passes the vote (the deadline for which he set as February 7th), he thinks that std.exception is the best place for it rather than it being in its own module.

- Jonathan M Davis

"Jonathan M Davis" <jmdavisProg@gmx.com> wrote in message news:mailman.535.1294662576.4748.digitalmars-d@puremagic.com...
>Okay, here's the new code and its documentation: http://is.gd/ktURa
>
>I followed Andrei's suggestion and merged most of the functions into a
>highly
>flexible assertPred. I also renamed the functions as suggested and
>attempted to
>fully document everything with fully functional examples instead of
>examples
>using types or functions which don't actually exist.
>
>So, now there's just assertThrown, assertNotThrown, collectExceptionMsg,
>and
>assertPred (though there are eight different overloads of assertPred). So,
>review
>away.
>
>From the sounds of it, if this code gets voted in, it'll be going into std.exception.

I like it. (I still think a way to make them collect an array of exceptions instead of throwing should make it's way in there at some point though.)

Couple small things though:

- The error message from the conditional version of assertPred should probably omit the two words "is not". That much is already clear from the fact that assertPred threw and says "failed:", and getting rid of it makes it much more readable as an expression: "failed: [hello] == [goodbye]" or "failed: [5] < [2]"

- The docs should have a section that gives an overview of assertPred by using a few simple examples of each of the assertPred uses. The way it is now, it's hard to see the overall logic behind the design of the assertPred's params since it's spread out all over. For instance:

---------------------------------------------
//Equivalent to assert(4 <= 5);
assertPred!"<="(4, 5);

//Equivalent to assert(1 * 2 == 2);
assertPred!"=="(1 * 2, 2);

//Equivalent to assert(7 + 5 == 12);
assertPred!"+"(7, 5, 12);

/*
   Equivalent to
   void func(T, U)(T lhs, U rhs)
   {
       auto result = lhs = rhs;

       assert(lhs == rhs);
       assert(result == rhs);
   }

   func(5, 7);
 */
assertPred!"opAssign"(5, 7);

/*
   Equivalent to
   void func(T, U, E)(T lhs, U rhs, E expected)
   {
       auto result = lhs += rhs;

       assert(lhs == expected);
       assert(result == expected);
   }

   func(5, 7, 12);
*/
assertPred!"-="(7, 5, 2);

assertPred!"*="(7, 5, 35);

//Equivalent to assert("hello " ~ "world" == "hello world");
assertPred!"~"("hello ", "world", "hello world");

//Equivalent to assert(1 == 1);
assertPred!"a == 1"(1);

//Equivalent to assert(2 * 3.0 == 6);
assertPred!"a * 3.0 == 6.0"(2);

//Equivalent to assert(42 == 42);
assertPred!"a == b"(42, 42);

//Equivalent to assert("hello " ~ "world" == "hello world");
assertPred!`a ~ b == "hello world"`("hello ", "world");

//Equivalent to assert((int[] range, int i){return canFind(range, i);}([1,
5, 7, 2], 7));
assertPred!((int[] range, int i){return canFind(range, i);})([1, 5, 7, 2],
7);

//Equivalent to assert((int a, int b, int c){return a == b && b == c;}(5, 5,
5));
assertPred!((int a, int b, int c){return a == b && b == c;})(5, 5, 5);

struct IntWrapper
{
    int value;

    int opCmp(const ref IntWrapper rhs) const
    {
        if(value < rhs.value)
            return -1;
        else if(value > rhs.value)
            return 1;

        return 0;
    }

    string toString()
    {
        return to!string(value);
    }
}

// Equivalent to assert(IntWrapper(0).opCmp(IntWrapper(6)) < 0);
assertPred!("opCmp", "<")(IntWrapper(0), IntWrapper(6));

// Equivalent to assert(IntWrapper(42).opCmp(IntWrapper(42)) == 0);
assertPred!("opCmp", "==")(IntWrapper(42), IntWrapper(42));
---------------------------------------------

Also notice that I cleaned up a few things, like eliminating the unnecessary struct constructor and making a few of the assert equivalents more clear.

Jonathan M Davis napisał:

> I followed Andrei's suggestion and merged most of the functions into a highly flexible assertPred. I also renamed the functions as suggested and attempted to fully document everything with fully functional examples instead of examples using types or functions which don't actually exist.

Did you zip the right file? I still see things like nameFunc and assertPlease.

On the whole the examples are too long. It's just daunting I can't see docs for *one* function without scrolling. Please give them a solid hair-cut -- max 10 lines with a median of 5. The descriptions are also watered down by over-explanatory writing.

> So, now there's just assertThrown, assertNotThrown, collectExceptionMsg, and assertPred (though there are eight different overloads of assertPred). So, review away.

Some suggestions:

assertPred:
Try putting expected in front; uniform call syntax can then set it apart from the operands:
assertPred!"%"(7, 5, 2); // old
2.assertPred!"%"(7, 5); // new

assertNotThrown: chain the original exception with AssertError as its cause? Oh, this one badly needs a real-life example.

assertThrown: I'd rather see generified collectException (call it collectThrown?). assertThrown may stay as a convenience wrapper, though.


Looking at the code I'm seeing the same cancerous coding style std.datetime suffered from (to a lesser extent, I admit).

For instance, this routine:

    if(result != expected)
    {
        if(msg.empty)
        {
            throw new AssertError(format(`assertPred!"%s" failed: [%s] %s [%s]: actual [%s], expected [%s].`,
                                         op,
                                         lhs,
                                         op,
                                         rhs,
                                         result,
                                         expected),
                                   file,
                                   line);
        }
        else
        {
            throw new AssertError(format(`assertPred!"%s" failed: [%s] %s [%s]: actual [%s], expected [%s]: %s`,
                                         op,
                                         lhs,
                                         op,
                                         rhs,
                                         result,
                                         expected,
                                         msg),
                                  file,
                                  line);
        }
    }

can be easily compressed to:

enforce(result==expected, new AssertError(
    format("[%s] %s [%s] failed: actual [%s], expected [%s]" ~ (msg.empty ? "." : ": %s"),
        op, lhs, op, rhs, result, expected, msg), file, line));

BTW, actual and expected should be in new lines directly under each other for eye-diffing (does wonders for long input): format("[%s] %s [%s] failed:\n[%s] - actual\n[%s] - expected" ~ ...

Another example:

{
        bool thrown = false;
        try
            assertNotThrown!AssertError(throwEx(new AssertError("It's an AssertError", __FILE__, __LINE__)), "It's a message");
        catch(AssertError)
            thrown = true;

        assert(thrown);
    }

can be:

    try {
        assertNotThrown!AssertError(throwEx(new AssertError("It's an AssertError", __FILE__, __LINE__)), "It's a message");
        assert(false);
    } catch(AssertError) { /*OK*/ }

and you don't have to introduce a new scope every time.

Not to mention that such routines recur in your code with little discrepancies, so abstracting out private helpers may pay off. Fixing such "readability bugs" is essential for a standard library module.

On the bright side, I do appreciate the thoroughness and extent of unittests in this module. Is coverage 100%?

> From the sounds of it, if this code gets voted in, it'll be going into std.exception.

Please don't rush the adoption. This module, albeit useful, still needs work.

-- 
Tomek

On Monday, January 10, 2011 13:48:50 Tomek Sowiński wrote:
> Jonathan M Davis napisał:
> > I followed Andrei's suggestion and merged most of the functions into a highly flexible assertPred. I also renamed the functions as suggested and attempted to fully document everything with fully functional examples instead of examples using types or functions which don't actually exist.
> 
> Did you zip the right file? I still see things like nameFunc and assertPlease.

??? Those are supposed to be there. All examples are tested in the unit tests exactly as they are.

> On the whole the examples are too long. It's just daunting I can't see docs for *one* function without scrolling. Please give them a solid hair-cut -- max 10 lines with a median of 5. The descriptions are also watered down by over-explanatory writing.

Perhaps. If I cut down on the examples though, the usage wouldn't be as clear. The idea was to be thorough. Andrei wanted better examples, so I gave better examples. However, it is a bit of a balancing act, and I may have put too many in. It's debatable. Nick's suggestion of a main description before each individual overload would help with that.

> > So, now there's just assertThrown, assertNotThrown, collectExceptionMsg, and assertPred (though there are eight different overloads of assertPred). So, review away.
> 
> Some suggestions:
> 
> assertPred:
> Try putting expected in front; uniform call syntax can then set it apart
> from the operands: assertPred!"%"(7, 5, 2); // old
> 2.assertPred!"%"(7, 5); // new

I really don't see any value to this.

1. You can't do that with assert, and assertPred is essentially supposed to be a fancy assert.

2. A number of assertPred overloads don't even have an expected, so it would be inconsistent.

3. People already are annoyed enough that the operator doesn't end up between the arguments. Putting the result on the left-hand side of the operator like that would make it that much more confusing.

> assertNotThrown: chain the original exception with AssertError as its cause? Oh, this one badly needs a real-life example.

I suppose that chaining it would be a good idea. I didn't think of that. But if you want examples, it's used in the unit tests in this very module, and I used it heavily in std.datetime.

> assertThrown: I'd rather see generified collectException (call it collectThrown?). assertThrown may stay as a convenience wrapper, though.

??? I don't get what you're trying for here. assertThrown isn't trying to collect exceptions at all. It's testing whether the given exception was thrown like it's supposed to be for the given function call. If it was, then the assertion succeeded. If it wasn't, then an AssertError is thrown. Just like assert.

> Looking at the code I'm seeing the same cancerous coding style std.datetime suffered from (to a lesser extent, I admit).
> 
> For instance, this routine:
> 
>     if(result != expected)
>     {
>         if(msg.empty)
>         {
>             throw new AssertError(format(`assertPred!"%s" failed: [%s] %s
> [%s]: actual [%s], expected [%s].`, op,
>                                          lhs,
>                                          op,
>                                          rhs,
>                                          result,
>                                          expected),
>                                    file,
>                                    line);
>         }
>         else
>         {
>             throw new AssertError(format(`assertPred!"%s" failed: [%s] %s
> [%s]: actual [%s], expected [%s]: %s`, op,
>                                          lhs,
>                                          op,
>                                          rhs,
>                                          result,
>                                          expected,
>                                          msg),
>                                   file,
>                                   line);
>         }
>     }
> 
> can be easily compressed to:
> 
> enforce(result==expected, new AssertError(
>     format("[%s] %s [%s] failed: actual [%s], expected [%s]" ~ (msg.empty ?
> "." : ": %s"), op, lhs, op, rhs, result, expected, msg), file, line));

I really have no problem with them being separate as they are. I think that I end up writing them that way because I see them as two separate code paths. It wouldn't necessarily be a bad idea to combine them, but I really don't think that it's a big deal.

> BTW, actual and expected should be in new lines directly under each other for eye-diffing (does wonders for long input): format("[%s] %s [%s] failed:\n[%s] - actual\n[%s] - expected" ~ ...

Good idea.

> Another example:
> 
> {
>         bool thrown = false;
>         try
>             assertNotThrown!AssertError(throwEx(new AssertError("It's an
> AssertError", __FILE__, __LINE__)), "It's a message"); catch(AssertError)
>             thrown = true;
> 
>         assert(thrown);
>     }
> 
> can be:
> 
>     try {
>         assertNotThrown!AssertError(throwEx(new AssertError("It's an
> AssertError", __FILE__, __LINE__)), "It's a message"); assert(false);
>     } catch(AssertError) { /*OK*/ }
> 
> and you don't have to introduce a new scope every time.

Doesn't work actually - at least not in the general case (for this particular test, it's arguably okay). It doesn't take into account the case where an exception other than AssertError is thrown. The exception escapes instead of hitting the assertion. I believe that they are the way they are because I was essentialy re-writing assertThrown to test assertThrown. Regardless, we're talking about the unit tests here, not the actual code, so I don't think that it's as big a deal.

> Not to mention that such routines recur in your code with little discrepancies, so abstracting out private helpers may pay off. Fixing such "readability bugs" is essential for a standard library module.
> 
> On the bright side, I do appreciate the thoroughness and extent of unittests in this module. Is coverage 100%?

I doubt it. I'm likely to have missed _something_. I haven't actually run a code coverage tool of any kind. IIRC, dmd -cov will do something along those lines, but I've never actually used it. Perhaps I should look into starting to use it. I just usually try and be really thorough.

> > From the sounds of it, if this code gets voted in, it'll be going into std.exception.
> 
> Please don't rush the adoption. This module, albeit useful, still needs work.

Well, Andrei set the vote deadline for February 7th, so it's not like it's being rushed. Regardless, the main question is the API. Things like whether the actual function bodies are as clear or terse as they maybe should be aren't as big a deal IMO (though not unimportant), because those can be fixed later if need be.

- Jonathan M Davis

On Monday, January 10, 2011 14:41:51 Jonathan M Davis wrote:
> On Monday, January 10, 2011 13:48:50 Tomek Sowiński wrote:
> > Another example:
> > 
> > {
> > 
> >         bool thrown = false;
> >         try
> > 
> >             assertNotThrown!AssertError(throwEx(new AssertError("It's an
> > 
> > AssertError", __FILE__, __LINE__)), "It's a message"); catch(AssertError)
> > 
> >             thrown = true;
> > 
> >         assert(thrown);
> > 
> >     }
> > 
> > can be:
> >     try {
> > 
> >         assertNotThrown!AssertError(throwEx(new AssertError("It's an
> > 
> > AssertError", __FILE__, __LINE__)), "It's a message"); assert(false);
> > 
> >     } catch(AssertError) { /*OK*/ }
> > 
> > and you don't have to introduce a new scope every time.
> 
> Doesn't work actually - at least not in the general case (for this particular test, it's arguably okay). It doesn't take into account the case where an exception other than AssertError is thrown. The exception escapes instead of hitting the assertion. I believe that they are the way they are because I was essentialy re-writing assertThrown to test assertThrown. Regardless, we're talking about the unit tests here, not the actual code, so I don't think that it's as big a deal.

Actually, I now remember the big reason that your version doesn't work at all. The AssertError thrown by assert(false); inside of the try block would just be caught by the catch block. So, it doesn't work at all. You have to do it pretty much the way that I did it. It was one of those nuances in writing assertNotThrown that initially missed when coming up with it in the first place. So, the ugly way is necessary.

- Jonathan M Davis

On 11/01/11 03:09, Jonathan M Davis wrote:
> On Monday 10 January 2011 05:40:39 Justin Johansson wrote:
>> On 10/01/11 23:29, Jonathan M Davis wrote:
>>>    From the sounds of it, if this code gets voted in, it'll be going into
>>>
>>> std.exception.
>>>
>>> - Jonathan M Davis
>>
>> May it be asked by what authority you can say that (ie. as said above)?
>
> Andrei's posts in this thread. Assuming that the code passes the vote (the
> deadline for which he set as February 7th), he thinks that std.exception is the
> best place for it rather than it being in its own module.

Oh okay.  Thanks for that; I missed Andrei's post in earlier this thread but found it now.  Good luck with the voting.  -- Justin

Jonathan M Davis napisał:

> On Monday, January 10, 2011 13:48:50 Tomek Sowiński wrote:
> > Jonathan M Davis napisał:
> > > I followed Andrei's suggestion and merged most of the functions into a highly flexible assertPred. I also renamed the functions as suggested and attempted to fully document everything with fully functional examples instead of examples using types or functions which don't actually exist.
> > 
> > Did you zip the right file? I still see things like nameFunc and assertPlease.
> 
> ??? Those are supposed to be there. All examples are tested in the unit tests exactly as they are.

I just thought "instead of examples using types or functions which don't actually exist" meant well-known Phobos functions would be used.

> > On the whole the examples are too long. It's just daunting I can't see docs for *one* function without scrolling. Please give them a solid hair-cut -- max 10 lines with a median of 5. The descriptions are also watered down by over-explanatory writing.
> 
> Perhaps. If I cut down on the examples though, the usage wouldn't be as clear. The idea was to be thorough. Andrei wanted better examples, so I gave better examples.

Not sure if longer means better.

> However, it is a bit of a balancing act, and I may have put too many in. It's debatable. Nick's suggestion of a main description before each individual overload would help with that.

I agree. Perhaps a synopsis for the whole module like in std.variant would help too.

> > > So, now there's just assertThrown, assertNotThrown, collectExceptionMsg, and assertPred (though there are eight different overloads of assertPred). So, review away.
> > 
> > Some suggestions:
> > 
> > assertPred:
> > Try putting expected in front; uniform call syntax can then set it apart
> > from the operands: assertPred!"%"(7, 5, 2); // old
> > 2.assertPred!"%"(7, 5); // new
> 
> I really don't see any value to this.
> 
> 1. You can't do that with assert, and assertPred is essentially supposed to be a fancy assert.
> 
> 2. A number of assertPred overloads don't even have an expected, so it would be inconsistent.
> 
> 3. People already are annoyed enough that the operator doesn't end up between the arguments. Putting the result on the left-hand side of the operator like that would make it that much more confusing.

OK, I understand.

> > assertNotThrown: chain the original exception with AssertError as its cause? Oh, this one badly needs a real-life example.
> 
> I suppose that chaining it would be a good idea. I didn't think of that. But if you want examples, it's used in the unit tests in this very module, and I used it heavily in std.datetime.

I meant a real-life example in documentation. People may often ask themselves "how is it different than !assertThrown()?".

> > assertThrown: I'd rather see generified collectException (call it collectThrown?). assertThrown may stay as a convenience wrapper, though.
> 
> ??? I don't get what you're trying for here. assertThrown isn't trying to collect exceptions at all. It's testing whether the given exception was thrown like it's supposed to be for the given function call. If it was, then the assertion succeeded. If it wasn't, then an AssertError is thrown. Just like assert.

I mean now collectException doesn't have a parametrized catch block like assertThrown does. If it did, the latter could come down to:

void assertThrown(T : Throwable = Exception, F)
                   (lazy F funcToCall, string msg = null, string file = __FILE__, size_t line = __LINE__)
{
	T e = collectThrown!T(funcToCall);
	if (e is null)
		throw new AssertError(...);
}

Shortening assertThrown's implementation is a bonus, main gain is better collectThrown().

[there's more down]

> > Looking at the code I'm seeing the same cancerous coding style std.datetime suffered from (to a lesser extent, I admit).
> > 
> > For instance, this routine:
> > 
> >     if(result != expected)
> >     {
> >         if(msg.empty)
> >         {
> >             throw new AssertError(format(`assertPred!"%s" failed: [%s] %s
> > [%s]: actual [%s], expected [%s].`, op,
> >                                          lhs,
> >                                          op,
> >                                          rhs,
> >                                          result,
> >                                          expected),
> >                                    file,
> >                                    line);
> >         }
> >         else
> >         {
> >             throw new AssertError(format(`assertPred!"%s" failed: [%s] %s
> > [%s]: actual [%s], expected [%s]: %s`, op,
> >                                          lhs,
> >                                          op,
> >                                          rhs,
> >                                          result,
> >                                          expected,
> >                                          msg),
> >                                   file,
> >                                   line);
> >         }
> >     }
> > 
> > can be easily compressed to:
> > 
> > enforce(result==expected, new AssertError(
> >     format("[%s] %s [%s] failed: actual [%s], expected [%s]" ~ (msg.empty ?
> > "." : ": %s"), op, lhs, op, rhs, result, expected, msg), file, line));
> 
> I really have no problem with them being separate as they are. I think that I end up writing them that way because I see them as two separate code paths. It wouldn't necessarily be a bad idea to combine them, but I really don't think that it's a big deal.
> 
> > Another example:
> > 
> > {
> >         bool thrown = false;
> >         try
> >             assertNotThrown!AssertError(throwEx(new AssertError("It's an
> > AssertError", __FILE__, __LINE__)), "It's a message"); catch(AssertError)
> >             thrown = true;
> > 
> >         assert(thrown);
> >     }
> > 
> > can be:
> > 
> >     try {
> >         assertNotThrown!AssertError(throwEx(new AssertError("It's an
> > AssertError", __FILE__, __LINE__)), "It's a message"); assert(false);
> >     } catch(AssertError) { /*OK*/ }
> > 
> > and you don't have to introduce a new scope every time.
> 
> Doesn't work actually - at least not in the general case (for this particular test, it's arguably okay). It doesn't take into account the case where an exception other than AssertError is thrown. The exception escapes instead of hitting the assertion. I believe that they are the way they are because I was essentialy re-writing assertThrown to test assertThrown.

OK, you're right. A generic collectThrown() would help also here, though.

> Regardless, we're talking about the unit tests here, not the actual code, so I don't think that it's as big a deal.

The actual code is also repetitive.

At work I just finished a task of realigning applications in the system to reflect recent changes in configuration database persistence. Our system is mainly written in two languages. One of them took the course of abstracting away persistence routines for all applications; reworking took a day. Applications in the other language were created by copy-pasting existing code and sculpting it to achieve desired functionality. So the very same persistence routines were written N times, each different due to varied intensity of maintenance and keeping up with changes in the DB schema over time. Reworking took nearly a month.

Copy-pasted code is bound to age badly and the only way to treat cancer is to kill it now before it grows. It *is* a big deal.

-- 
Tomek

On Tuesday, January 11, 2011 12:25:53 Tomek Sowiński wrote:
> Jonathan M Davis napisał:
> > On Monday, January 10, 2011 13:48:50 Tomek Sowiński wrote:
> > > Jonathan M Davis napisał:
> > > > I followed Andrei's suggestion and merged most of the functions into a highly flexible assertPred. I also renamed the functions as suggested and attempted to fully document everything with fully functional examples instead of examples using types or functions which don't actually exist.
> > > 
> > > Did you zip the right file? I still see things like nameFunc and assertPlease.
> > 
> > ??? Those are supposed to be there. All examples are tested in the unit tests exactly as they are.
> 
> I just thought "instead of examples using types or functions which don't actually exist" meant well-known Phobos functions would be used.

Well, that would be better, but at least when it comes to types, that doesn't work. Not only is Phobos generally lacking in types, but some of the examples which show what a typical error message from the functions would look like require incorrectly implemented types. I might be able to use existing functions for the examples using functions though.

> > > assertThrown: I'd rather see generified collectException (call it collectThrown?). assertThrown may stay as a convenience wrapper, though.
> > 
> > ??? I don't get what you're trying for here. assertThrown isn't trying to collect exceptions at all. It's testing whether the given exception was thrown like it's supposed to be for the given function call. If it was, then the assertion succeeded. If it wasn't, then an AssertError is thrown. Just like assert.
> 
> I mean now collectException doesn't have a parametrized catch block like assertThrown does. If it did, the latter could come down to:
> 
> void assertThrown(T : Throwable = Exception, F)
>                    (lazy F funcToCall, string msg = null, string file =
> __FILE__, size_t line = __LINE__) {
> 	T e = collectThrown!T(funcToCall);
> 	if (e is null)
> 		throw new AssertError(...);
> }
> 
> Shortening assertThrown's implementation is a bonus, main gain is better
> collectThrown().
> 
> [there's more down]
> 
> > > Looking at the code I'm seeing the same cancerous coding style std.datetime suffered from (to a lesser extent, I admit).
> > > 
> > > For instance, this routine:
> > >     if(result != expected)
> > >     {
> > > 
> > >         if(msg.empty)
> > >         {
> > > 
> > >             throw new AssertError(format(`assertPred!"%s" failed: [%s]
> > >             %s
> > > 
> > > [%s]: actual [%s], expected [%s].`, op,
> > > 
> > >                                          lhs,
> > >                                          op,
> > >                                          rhs,
> > >                                          result,
> > >                                          expected),
> > > 
> > >                                    file,
> > >                                    line);
> > > 
> > >         }
> > >         else
> > >         {
> > > 
> > >             throw new AssertError(format(`assertPred!"%s" failed: [%s]
> > >             %s
> > > 
> > > [%s]: actual [%s], expected [%s]: %s`, op,
> > > 
> > >                                          lhs,
> > >                                          op,
> > >                                          rhs,
> > >                                          result,
> > >                                          expected,
> > >                                          msg),
> > > 
> > >                                   file,
> > >                                   line);
> > > 
> > >         }
> > > 
> > >     }
> > > 
> > > can be easily compressed to:
> > > 
> > > enforce(result==expected, new AssertError(
> > > 
> > >     format("[%s] %s [%s] failed: actual [%s], expected [%s]" ~
> > >     (msg.empty ?
> > > 
> > > "." : ": %s"), op, lhs, op, rhs, result, expected, msg), file, line));
> > 
> > I really have no problem with them being separate as they are. I think that I end up writing them that way because I see them as two separate code paths. It wouldn't necessarily be a bad idea to combine them, but I really don't think that it's a big deal.
> > 
> > > Another example:
> > > 
> > > {
> > > 
> > >         bool thrown = false;
> > >         try
> > > 
> > >             assertNotThrown!AssertError(throwEx(new AssertError("It's
> > >             an
> > > 
> > > AssertError", __FILE__, __LINE__)), "It's a message");
> > > catch(AssertError)
> > > 
> > >             thrown = true;
> > > 
> > >         assert(thrown);
> > > 
> > >     }
> > > 
> > > can be:
> > >     try {
> > > 
> > >         assertNotThrown!AssertError(throwEx(new AssertError("It's an
> > > 
> > > AssertError", __FILE__, __LINE__)), "It's a message"); assert(false);
> > > 
> > >     } catch(AssertError) { /*OK*/ }
> > > 
> > > and you don't have to introduce a new scope every time.
> > 
> > Doesn't work actually - at least not in the general case (for this particular test, it's arguably okay). It doesn't take into account the case where an exception other than AssertError is thrown. The exception escapes instead of hitting the assertion. I believe that they are the way they are because I was essentialy re-writing assertThrown to test assertThrown.
> 
> OK, you're right. A generic collectThrown() would help also here, though.

I still don't get the point of collectThrown() and what such a function would do. It just looks like you're wrapping a try-catch block in a function and returning the exception which was thrown instead of putting the try-catch block in the function calling collectThrown. It seems pointless.

The whole point of assertThrown is to test that a specific exception type was thrown from the given function call. If it was, then nothing happens. If it wasn't, then an AssertError is thrown. Any other exceptions are irrelevant and will be propagated normally.

The whole point of assertNotThrown is to test that no exception of a specific type was thrown from the given function call. If none was thrown, then nothing happens. If one _was_ thrown, then an AssertError is thrown. Any other exceptions are irrelevant and will be propagated normally.

The idea is to use them to test that a function throws the exceptions that it's supposed  to throw when it's supposed to throw them and that it doesn't throw those exceptions when it's not supposed to.

> > Regardless, we're
> > talking about the unit tests here, not the actual code, so I don't think
> > that it's as big a deal.
> 
> The actual code is also repetitive.
> 
> At work I just finished a task of realigning applications in the system to reflect recent changes in configuration database persistence. Our system is mainly written in two languages. One of them took the course of abstracting away persistence routines for all applications; reworking took a day. Applications in the other language were created by copy-pasting existing code and sculpting it to achieve desired functionality. So the very same persistence routines were written N times, each different due to varied intensity of maintenance and keeping up with changes in the DB schema over time. Reworking took nearly a month.
> 
> Copy-pasted code is bound to age badly and the only way to treat cancer is to kill it now before it grows. It *is* a big deal.

While I agree that refactoring code so that you have a separate function call instead of copy-pasting code is definitely a good idea and that it's very poor practice to just copy-paste everything everwhere, I don't agree that some level of duplication _within_ a function is necessarily a problem - particularly if the resulting code is harder to understand, which does tend to happen if you're overly zealous in trying to eliminate duplicate code. If code duplication can be reasonably removed within a function without harming readability, then it's likely a good idea to do it. And perhaps I don't pay enough attention to removing code duplication within functions sometimes, but I don't believe that any of the code that's duplicated here makes any sense being split out into separate functions, so I don't think that it's a big deal. However, I will look at reducing the code duplication in a readable manner. I do, however, rate code legibility as more important than removing code duplication within a function, and doing stuff like heavily stuff like unaryFun or binaryFun within a function just to avoid having separate static if blocks is not the sort of thing that I'm likely to consider better in many cases, though I get the impression that you likely would (I don't think that that really applies to these particular functions though).

- Jonathan M Davis