[Kmymoney-devel] Review Request 109803: Add CSV export capability.

[Allan Anderson]

> On April 24, 2013, 8 p.m., Thomas Baumgart wrote:
> > kmymoney/plugins/csvexport/csvexportdlg.h, line 68
> > <http://git.reviewboard.kde.org/r/109803/diff/4/?file=140779#file140779line68>
> >
> >     You should not make member vars public. Simply move those two declarations to the bottom where all the other member var m_idList is declared.

Done.  Needed also to add a getter for m_accountId, for access from csvwriter.cpp.


> On April 24, 2013, 8 p.m., Thomas Baumgart wrote:
> > kmymoney/plugins/csvexport/csvexportdlg.cpp, line 222
> > <http://git.reviewboard.kde.org/r/109803/diff/4/?file=140780#file140780line222>
> >
> >     This sorting still happens inside the while loop. Or am I missing sth. here?

No, it's not you - 'tis I.  Apologies for my tunnel vision.


> On April 24, 2013, 8 p.m., Thomas Baumgart wrote:
> > kmymoney/plugins/csvexport/csvexporterplugin.h, line 45
> > <http://git.reviewboard.kde.org/r/109803/diff/4/?file=140782#file140782line45>
> >
> >     Public member vars

Not sure how to go about this as it is accessed from outside its class.  Contemplated using protected and friend, which I see are used elsewhere, but reading about, it's not always deemed to be good practice.  Or, use a getter here too?


> On April 24, 2013, 8 p.m., Thomas Baumgart wrote:
> > kmymoney/plugins/csvexport/csvwriter.cpp, line 81
> > <http://git.reviewboard.kde.org/r/109803/diff/4/?file=140785#file140785line81>
> >
> >     Not sure if debug messages need to be translated. This is debatable.

Hmm...  Not sure its use is as debug, but rather more informative.  It was a last-minute change.


> On April 24, 2013, 8 p.m., Thomas Baumgart wrote:
> > kmymoney/plugins/csvexport/csvwriter.cpp, line 101
> > <http://git.reviewboard.kde.org/r/109803/diff/4/?file=140785#file140785line101>
> >
> >     why not using "\n\n" instead? Your way needs one more byte of mem plus cpu time during each call.

Why not, indeed?  Just harking back to your comment about using QLatin1String/Char, was that just instead of Qstring("blahblah"), or also for 
"blah" + "blah1"?  The article left me in doubt.


> On April 24, 2013, 8 p.m., Thomas Baumgart wrote:
> > kmymoney/plugins/csvexport/csvwriter.cpp, line 190
> > <http://git.reviewboard.kde.org/r/109803/diff/4/?file=140785#file140785line190>
> >
> >     Here's another way to do this:
> >     
> >       str += QString("\"%1\",").arg(payee.name());
> >     
> >     Don't know which one is better.

I'll sleep on that.


- Allan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109803/#review31509
-----------------------------------------------------------


On April 24, 2013, 4:16 p.m., Allan Anderson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109803/
> -----------------------------------------------------------
> 
> (Updated April 24, 2013, 4:16 p.m.)
> 
> 
> Review request for KMymoney.
> 
> 
> Description
> -------
> 
> Add CSV export capability.  Modify existing KMyMoney File menu in order to make menu item positions more logical.
> 
> 
> This addresses bug 317614.
>     http://bugs.kde.org/show_bug.cgi?id=317614
> 
> 
> Diffs
> -----
> 
>   kmymoney/kmymoneyui.rc f353641 
>   kmymoney/plugins/CMakeLists.txt 81ca458 
>   kmymoney/plugins/csvexport/CMakeLists.txt PRE-CREATION 
>   kmymoney/plugins/csvexport/csvexportdlg.h PRE-CREATION 
>   kmymoney/plugins/csvexport/csvexportdlg.cpp PRE-CREATION 
>   kmymoney/plugins/csvexport/csvexportdlg.ui PRE-CREATION 
>   kmymoney/plugins/csvexport/csvexporterplugin.h PRE-CREATION 
>   kmymoney/plugins/csvexport/csvexporterplugin.cpp PRE-CREATION 
>   kmymoney/plugins/csvexport/csvwriter.h PRE-CREATION 
>   kmymoney/plugins/csvexport/csvwriter.cpp PRE-CREATION 
>   kmymoney/plugins/csvexport/kmm_csvexport.desktop PRE-CREATION 
>   kmymoney/plugins/csvexport/kmm_csvexport.rc PRE-CREATION 
>   kmymoney/plugins/csvimport/investprocessing.cpp 07d4d82 
>   kmymoney/plugins/csvimport/kmm_csvimport.rc d678169 
> 
> Diff: http://git.reviewboard.kde.org/r/109803/diff/
> 
> 
> Testing
> -------
> 
> Exported numerous checking and investment CSV files, and then imported into KMyMoney via CSV import (discovering a few minor issues in the existing KMyMoney accounts in the process.)
> Ran astyle and Krazy2.
> 
> 
> Thanks,
> 
> Allan Anderson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kmymoney-devel/attachments/20130424/9d09a4f3/attachment.html>