[Kmymoney-devel] Re: Review Request: csvplugin update to add additional unit test, and improve field delimiter handling

[Allan Anderson]

> On 2010-10-14 11:36:15, Thomas Baumgart wrote:
> > /trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/investprocessing.cpp, line 56
> > <http://svn.reviewboard.kde.org/r/5463/diff/1/?file=38583#file38583line56>
> >
> >     Missing call of base class ctor here.

Sorry, but I'm not sure what you mean here.


> On 2010-10-14 11:36:15, Thomas Baumgart wrote:
> > /trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/investprocessing.cpp, line 215
> > <http://svn.reviewboard.kde.org/r/5463/diff/1/?file=38583#file38583line215>
> >
> >     This only modifies 10 widgets whereas enableInputs modifies 17.
> >     
> >     Better use a single method and pass it true/false as parameter, eg.
> >     
> >     enableInputs(bool enable) {...}
> >     
> >     disableInputs() { enableInputs(false); }
> >     
> >     enableInputs() { enableInputs(true); }
> >     
> >     This avoids the error prone job of maintaining two lists.

Yes, I can see that would be better, although there were other minor differences between the two.
However, I took a different tack and have removed disableInputs() altogether.  It wasn't really achieving anything.  I have the columns all disabled in the UIs, together with other fields which depend on the data read in.  So, they only get enabled once a file is read in, so that the user doesn't waste effort selecting before he's seen the data.  There's not really any need to disable them after that, the hope being that the user will have twigged!  In fact, I've gone slightly further, having found from experience that if a file is imported and a wrong date format has been detected, the user will just either want to select the correct format, or correct the start or end lines because he has included non-financial data, ie a header or trailer line.  So there was no point in disabling the start line selection and forcing him to reload the file.


- Allan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5463/#review8146
-----------------------------------------------------------


On 2010-09-27 16:53:42, Allan Anderson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5463/
> -----------------------------------------------------------
> 
> (Updated 2010-09-27 16:53:42)
> 
> 
> Review request for kmymoney.
> 
> 
> Summary
> -------
> 
> The initial intention was to add an additional unit test, which involved removing data line parsing from the displayLine() routine into a small separate class to enable access from the unit test routine.  This led to benefits in two other areas.  
> 
> One csv test file I have had a comma as thousand separator, which was conflicting with the comma field separator.  Initially, I looked at this from the point of view of field separators and concocted a separator which successfully dealt with the issue.  However, I realised that the same odd separator might not work in another similar situation.  So, I've improved the detection and handling of this issue.
> 
> This then led to changes in csvProcessing() and investProcessing(), removing redundant code and improving efficiency slightly.
> 
> Two UIs have had a now unneeded field separator combobox item removed.  Some minor purely cosmetic changes also have been made.
> 
> 
> Diffs
> -----
> 
>   /trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/CMakeLists.txt 1180212 
>   /trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/csvimporterdlgdecl.ui 1180212 
>   /trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/csvprocessing.h 1180212 
>   /trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/csvprocessing.cpp 1180212 
>   /trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/csvutil.h PRE-CREATION 
>   /trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/csvutil.cpp PRE-CREATION 
>   /trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/investmentdlgdecl.ui 1180212 
>   /trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/investprocessing.h 1180212 
>   /trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/investprocessing.cpp 1180212 
>   /trunk/extragear/office/kmymoney/kmymoney/plugins/csvimport/redefinedlg.cpp 1180212 
> 
> Diff: http://svn.reviewboard.kde.org/r/5463/diff
> 
> 
> Testing
> -------
> 
> The new parsedatatest() unit test runs successfully.  Krazy and astyle have been run. Functional testing done with various csv format files.
> 
> 
> Thanks,
> 
> Allan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kmymoney-devel/attachments/20101015/d111f69e/attachment.htm