some csv feedback

[Christian Stimming]

Hi Benjamin,

I've just looked more closely into your code and I thought I'd give you some 
feedback.

First of all, all of your code looks very good! You really spent a lot of work 
into separating the different parts of the task into separate functions, and 
this makes it really easy to understand what you're doing. Very good!

Also, you document really a lot of places in between, but without overly 
distracting comments. That way, even from reading the code one can gather 
quite clearly the idea of what is going on. And I'm very sure this whole 
style of clearly structuring and documenting your code will already pay off 
for yourself once you have to look at code parts of yourself that are some 
months old. Also very good!

(I have to admit I haven't been able to compile your branch due to goffice 
issues in lib/stf, but I'm quite sure I'll figure these out soon.)

When looking into the glade dialog, I'm a bit surprised about the multitude of 
checkboxes for the possible field separators. I think this is probably an 
area where you might consider different widget(s) eventually (a list view 
with multiple selections possible?), but for now this should be fine.

I have some smaller suggestions on how you could improve your code even more. 
I encountered some compile warnings:

gnc-csv-model.c: In function 'gnc_parse_to_trans':
gnc-csv-model.c:485: warning: 'begin_error_lines' may be used uninitialized in 
this function
gnc-csv-model.c:485: warning: 'error_lines' may be used uninitialized in this 
function
gnc-csv-model.c:487: warning: 'last_transaction' may be used uninitialized in 
this function

Did you specify any particular options at ./configure time? I'd suggest to use 
the following:  --enable-error-on-warning --enable-compile-warnings
In my experience, even these "maybe used uninitialized" warnings provide quite 
a helpful hint on which variables should indeed get an initialization value. 
But apart from this particular case I'd suggest to compile with the 
error-on-warning and compile-warnings all the time, because it avoids quite 
some hassle once someone with a different compiler tries to use this.

Then, I have a conceptual addition with dates. Do you already have plans for 
different date separators? In the struct date_format_internal there is 
only '/'.  Eventually, the code needs to take several different date 
separators into account. I know of 2007/06/05 and 2007-06-05 and 05.06.2007, 
i.e. the separator is one of [/-.], but there might be others. Maybe 
http://en.wikipedia.org/wiki/Date_and_time_notation_by_country tells of some, 
but maybe that page is just a prime example of the wiki principle failing 
gloriously. Heh :-) 

Then, I have some suggestions from the MS Windows front. You are using some 
unix-specific file handling system calls. Although those are fine on Unix, 
they don't carry over well to MS Windows, and in almost any case the glib 
library has some more general function because of that.  In particular, I 
would suggest these replacements:
stat() -> g_file_test() for testing for existance.
mmap() -> g_mapped_file_new() ; use g_mapped_file_get_length() to get the 
length instead of stat(). See  
http://developer.gnome.org/doc/API/2.0/glib/glib-File-Utilities.html

And eventually I saw some instances of g_malloc(sizeof()) in your code... I 
think as we are now in glib time we shouldn't have to deal with the sizeof() 
anymore, and I'd suggest the following replacement:
Foo *x = g_malloc(sizeof(Foo))  ->  Foo *x = g_new(Foo, 1) // or even g_new0()

Nevertheless, these suggestions concern really only a very small portion of 
what you already contributed. I think your structure and your coding style 
will enable you to achieve a very good outcome and really implement a very 
cool feature. Way to go!

Regards,

Christian