some csv feedback

[Benjamin Sperisen]

Thanks for all of your comments!

Christian and I already discussed some of his email in #gnucash, but
I'll summarize that discussion here, and respond to the rest of his
comments as well as those from Derek and Thomas about the date
parsing.

On 7/8/07, Christian Stimming <stimming at tuhh.de> wrote:
> (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.)

I just committed some changes that should make the lib/stf code
compile with goffice 0.2, but if anyone still has problems, please let
me know. For now, I've commented out the calls to functions that are
in goffice-0.3, and I'll uncomment them eventually once goffice-0.3 is
common in distros. However, it would be nice if I could use some kind
of macro to do this for me, such as

#ifdef GOFFICE_VERSION_0_3
goffice_0_3_call();
#else
replacement_code();
#endif

Is there anything like this?

> 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.

The checkboxes are just an imitation of the import feature in
Gnumeric. I agree, eventually this should be changed to something a
bit easier.

> 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

These should be gone now. However, I cannot get any uninitialized
warnings to show up for me, because I am passing the following
warnings to gcc:

CFLAGS =  -Werror -Wdeclaration-after-statement -Wno-pointer-sign
-D_FORTIFY_SOURCE=2 -Wall  -g  -O2 -Wall -Wunused -Wmissing-prototypes
-Wmissing-declarations  -Wno-unused

I am not sure how to get -Wall to appear after the -Wno-unused flag.
Christian mentioned that GNOME_COMPILE_WARNINGS is somehow adding that
in and that depends on the version of GNOME I'm running. I am running
2.18.1 (in Ubuntu 7.04); does anyone know how to fix this?

> 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.

I've now replaced those calls with the glib equivalents; it also has
added benefit of chopping out quite a bit of code!

> 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()

Okay, I've replaced all those calls.

On 7/10/07, ipwizard at users.sourceforge.net
<ipwizard at users.sourceforge.net> wrote:
> On Tuesday 10 July 2007 00:53, Derek Atkins wrote:
> > Christian Stimming <stimming at tuhh.de> writes:
> > > 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 :-)
> >
> > The QIF code has a decent date parser, although it's in scheme
> > right now.  The regex looks like:
> >
> >   "^ *([0-9]+) *[-/.'] *([0-9]+) *[-/.'] *([0-9]+).*$|^
> > *([0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]).*$"
> >
> > In other words the seperators are a dash (-), slash (/), period (.),
> > and single quote (').  Also note that there's no requirement that you
> > use the same separator both times.  E.g. you could use 7/1'07 and this
> > is a perfectly valid date.
>
> That's true, but 7/1'07 has a different meaning than 7/1/07 esp. if bot
> versions are used in the same file. This is important if you try to import
> ancient data from the last century. Also, there is no consitant usage of the
> apostrophe version among the various products even if they come from the same
> manufacturer (Quicken in this case).

Thomas, sorry, could you clarify what the difference between 7/1'07
and 7/1/07 is for me? Is it a distinction between the years 2007 and
1907?

If so, is it actually used in that way frequently? If it is, then I
guess we probably have to specify separators. Otherwise, is it
reasonable to simply guess at the year if only given two digits (as I
suspect the QIF date parser does)? I really do like Derek's
recommendation to not ask the user for a separator, as it makes the
user interface simpler (not to mention the code ;-), so if it's
possible to go that way, I'm all for it.

Benny