Warren Young wrote:
> Rick Gutleber wrote:
>>
>> 1. Does the new insertfrom( ) method and the InsertPolicy and
>> SQLStream objects require anything new in the tests? The tests are
>> very concise; I don't know if they are meant to be exhaustive.
>
> The MySQL++ test suite's concision reflects its immaturity, not a
> design goal. There should be a test for SQLStream at least in tests/*
> and code to exercise insertfrom() in examples/*.
> For the example, maybe something that reads a tab-delimited file of
> 'stock' items, makes a vector of SSQLSes from this, then inserts that
> into the DB?
Sounds good.
>
>> 2. Do I need to make additions to the user manual to reflect these
>> new pieces? Or is there someone else who handles documentation changes?
>
> Leave userman to me.
Gladly.
>> I attached the patch but if there is anything else I need to do, I
>> will make those changes and resubmit.
>
> Yeah, it needs some style fixes.
>
> 1. I don't see that splitting insertfrom() into public and private
> halves buys much. Multiple 'success = false' lines isn't any less
> redundant than multiple 'ac.rollback()' lines. (Not that I think
> you'll have too many method calls on 'ac'...most of them should be at
> the top and bottom of the function.) You also end up with an empty
> transaction if the insert range is empty, due to starting the
> transaction way up high like that, before you test the iterator
> range. If I'm wrong, tell me why.
You're right. I hadn't considered the possibility of an empty
transaction. I'm happy to merge them back to one function.
> 2. In the code, use tabs instead of spaces, size 4 [*], and lint your
> patch for other pointless whitespace changes. This accounts for about
> half your patch. As far as I can tell, 100% of the changes to
> lib/query.cpp are of this type, but it happens all through the patch.
> As a result, I haven't really studied the patch carefully. Too much
> noise masking the signal.
Ah, tabs, the bane of my existence, which is why I made a point to never
use them about, oh, 20 years ago. I realized there were whitespace
changes in the patch, some of which seemed to have been caused by
trailing white space on lines (which I have my editor set to strip) in
addition to tabs vs. spaces. I redid my changes to eliminate some of
it, but should have done more.
> 3. Take the unistd.h out of examples/cpool.cpp. I fixed that problem
> in svn a different way. Besides, it's unrelated to this patch.
> (Reading through your patch before submitting it would have caught #2,
> too.)
OK.
>
> 4. Say 'svn add lib/sqlstream.* lib/insertpolicy.h' before making the
> next patch. These files are missing from the one you just sent.
Oops. Duh.
>
> 5. Use 0 instead of NULL. (Just style.)
I had it that way at first and decided for some reason to change it. I
wasn't sure what the style was.
>
> 6. Don't cuddle 'else'.
But it's lonely! That's a HARD habit to break. I must have missed some.
>
> [*] Yes, we do use spaces instead of tabs in all of the documentation
> files. I've configured my text editor to switch whitespace modes
> based on the file type, so I don't often notice the change any more.
> We may at some point go with spaces across the board, but for the
> nonce, follow the current convention.
I also wasn't sure what the convention was because mysql++.bkl didn't
have tabs (actually it has a couple, but mostly not). I suppose I
should have just asked.
