review for unittests

[Jonathan M Davis]

On Saturday 29 January 2011 11:46:55 Andrei Alexandrescu wrote:
> I made one more pass through the unittests library and it looks fine,
> but I still find it wanting. Overall:
> 
> * From too few examples now it has too many! The fine line is somewhere
> in between (I'd say on the short side). I complained about the initial
> documentation because the example were meaningless (e.g. they used names
> that weren't defined. The current iteration of the library gives so many
> examples it's patronizing. Yes, if you give the list of operators
> supported, you don't need to give an example of each! We're not
> oligophrenes over here.
> 
> * To add insult to injury, examples have a double-spacing that is a bit
> much.

I thought that I'd removed all of that. Apparently not.

> * The code has a lot of lines beyond 80 characters. 80 characters should
> be plenty to write good code, and makes for readable code that doesn't
> need to occupy one's entire screen. I wish I could somehow convince
> everybody to not complain, not debate, and not show me the old style
> guide. Let's just write 80-columns code. That includes documentation.
> Please. Let's.
>
> * Even examples are sometimes too wide - some don't fit in my browser
> unless I enlarge it considerably.

You mean 80 lines of code in the examples or in the actual code? I'm fine with 80 
columns for examples, but I'll go insane if we start requiring 80 columns for 
normal code. That starts getting restrictive and ugly _really_ fast. You're 
quickly forced to put the next part of a statement on the next line just because 
you're hitting the 80 character limit instead of properly lining things up, and 
it makes them much harder to read.

As for the documentation, I made sure that the examples fit in the grey code 
boxes, and that's what I was aiming for. If that doesn't do it and restricting 
them to 80 columns does, then I'll do that.

> * I don't mean to belittle the effort, but the documentation needs one
> more pass for typos etc. I wouldn't mention this if I hadn't noticed
> Jonathan is otherwise a perfect speller and a good writer. So, it's
> "Generally speaking" not "Generally-speaking", "given set" not "give
> set"... and that's just the first line. Eliminate all flowery: there's
> no need for "generally speaking" and "in essence" "particularly" etc.
> etc. etc.
> 
> * For inline code use $(D ) not anything else

I'll make another pass then. I obviously missed some stuff. Feel free to point 
out spelling mistakes and the like. If they're there, it's pretty much a 
guarantee that it's because I missed them than because I didn't know better 
(though for some reason, I did get it in my head that it was generally-speaking 
rather than generally speaking; it does appear that you're correct however).

> * I appreciate the date-based examples but I'd rather stick with one
> focus at a time.

I used them so that I could give examples involving functions throwing 
exceptions without having to create types or functions in the examples which did 
that.

> * There's got to be too much of a good thing. The massive unittests, the
> lavish waste of vertical space (import with commas, anyone?) and the
> documentation make adding four simple concepts a 1887 lines deal. I
> guess that's borderline okay but I am increasingly hawkish about massive
> additions of small functionality to Phobos.

I confess that I hate having imports put together in one statement just like I 
hate declaring multiple variables on one line. I think that it harms readability 
and maintainability. But if you insist on putting imports on a single line in 
Phobos, then I can do that.

As for the unit tests, what do you want me to do? They verify that the functions 
work correctly. They're thorough. That naturally makes them longer rather than 
shorter. You pretty much either get short unit tests or thorough unit tests. I 
tend to err on the side of thorough rather than short.

> * The documentation for the mock-up assertPred could be moved up to the
> module documentation, which has the advantage that it avoids any
> self-explaining: "There is further documentation for each version below
> (this particular version of the function doesn't actually exist - it's
> here so that there's a good spot to put documentation for the function
> as a whole)." Why would anyone need to absorb that kind of information
> in order to use four simple concepts?

I'm all for finding ways to better organize the assertPred documentation. The 
general problem with it is that while it's quite simple to use, it has so many 
variations that it takes a fair bit of explaining. I'm not about to claim that 
it currently does it the best way that it could though.

> My vote is in favor to adopting this library, contingent upon (a) making
> a spelling and correctness pass and (b) giving the documentation a
> thorough haircut. The rest of my comments are optional as they could be
> effected later.

I'll make another pass at at the documentation and see if I can hit a better 
balance between too little explanation and not enough. In fact, I think that 
I'll just merge it into std.exception (since you seemed to think that that would 
be the place to put it) and post what it would then look like once put into 
Phobos (since doing stuff like putting assertPred documentation in the module 
comment would affect the module as a whole and std.exception obviously already 
has stuff in it). I'll take care of that and get it up within the next few days.

- Jonathan M Davis