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

[Allan Anderson]

> 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
> 
> Allan Anderson wrote:
>     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?
> 
> Thomas Baumgart wrote:
>     Yes, that's what they are there for.

Done.


> 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.
> 
> Allan Anderson wrote:
>     I'll sleep on that.
> 
> Marko Käning wrote:
>     Thomas' suggestion of using arguments in strings is used in many places in KMM.
> 
> Thomas Baumgart wrote:
>     Is it because Thomas (me in this case) wrote so much of that code? SCNR.

Swapped to this method as visually more compact.


> 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.
> 
> Allan Anderson wrote:
>     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.
> 
> Thomas Baumgart wrote:
>     Concatenation using either + or % uses runtime. Having a fixed string does not. I agree, it is just a small portion of cpu time but things like that add up and one should always have an eye on them.

Yes, I'm OK so far as concatenation is concerned.  Also, I'm OK so far as QString("blahblah") being replaced by QLatin1String("blahblah").  What I was unclear about was whether the string "blah" should be QLatin1String("blah")?  I get the impression that both are OK, but that QLatin1String is for the sake of consistency.


- 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/20130504/1a668f85/attachment-0001.html>