List:MySQL++« Previous MessageNext Message »
From:Rick Gutleber Date:November 10 2008 3:20pm
Subject:Re: insertfrom( ) questions
View as plain text  
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.


Thread
insertfrom( ) questionsRick Gutleber7 Nov
  • Re: insertfrom( ) questionsWarren Young8 Nov
    • 'result' is not a member of mysqlppJJ Harrison8 Nov
      • Re: 'result' is not a member of mysqlppWarren Young8 Nov
    • Re: insertfrom( ) questionsRick Gutleber10 Nov
      • Re: insertfrom( ) questionsWarren Young11 Nov
        • Re: insertfrom( ) questionsRick Gutleber11 Nov
          • Re: insertfrom( ) questionsWarren Young11 Nov
            • Re: insertfrom( ) questionsRick Gutleber12 Nov
    • Re: insertfrom( ) questionsRick Gutleber13 Nov
      • Re: insertfrom( ) questionsWarren Young13 Nov
        • Re: insertfrom( ) questionsRick Gutleber13 Nov
          • Re: insertfrom( ) questionsWarren Young13 Nov
  • RE: insertfrom( ) questionsIan Daysh11 Nov
    • Re: insertfrom( ) questionsJim Graf11 Nov