> On Nov. 26, 2012, 4:56 p.m., Cristian Oneț wrote: > > The patch looks really good, I still have to give it a try though until > > that I thought I'll publish these 'administrative' observations with the > > last one of them being: did you run astyle-kmymoney.sh (in the source tree, > > needs astyle 1.23 and nromalize from qt - search the mailinglist) before > > creating this patch? > > > > I hope you don't find this too restraining, as I've said the patch is > > really nice we just need to it to match the current style. I'm just buying > > some time with this to get a chance to test it better ;). > > > >
Regarding astyle-kmymoney.sh - looks like you're really buying yourself some time ;) since neither normalize nor astyle 1.23 come easy to get. I ran astyle 2.01 instead, configured like in the astyle-kmymoney.sh. Will that suffice? > On Nov. 26, 2012, 4:56 p.m., Cristian Oneț wrote: > > kmymoney/converter/mymoneystatementreader.h, line 27 > > <http://git.reviewboard.kde.org/r/107137/diff/1/?file=92953#file92953line27> > > > > Why were these header group markers removed? I've seen KMM files without the markers, so I assumed these are optional. That's why I followed my personal opinion on this - not to use such code beautifiers. > On Nov. 26, 2012, 4:56 p.m., Cristian Oneț wrote: > > kmymoney/converter/mymoneystatementreader.h, line 45 > > <http://git.reviewboard.kde.org/r/107137/diff/1/?file=92953#file92953line45> > > > > Project includes should use quotes. I know that it works either way but > > it's nicer that way. Ok, I'll fix it for all KMM includes from the patch. > On Nov. 26, 2012, 4:56 p.m., Cristian Oneț wrote: > > kmymoney/dialogs/existingtransactionmatchfinder.h, line 23 > > <http://git.reviewboard.kde.org/r/107137/diff/1/?file=92956#file92956line23> > > > > Try to use the same style for new headers as the old one if possible. > > The KDE 'Krazy' Checker can help check if the headers have the proper > > license. You mean file headers, not the "#include <QPair>", right? Ok, I can fix that. > On Nov. 26, 2012, 4:56 p.m., Cristian Oneț wrote: > > kmymoney/converter/mymoneystatementreader.cpp, line 64 > > <http://git.reviewboard.kde.org/r/107137/diff/1/?file=92954#file92954line64> > > > > Same as above. Will do. - Łukasz ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107137/#review22575 ----------------------------------------------------------- On Oct. 30, 2012, 5:42 p.m., Łukasz Maszczyński wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/107137/ > ----------------------------------------------------------- > > (Updated Oct. 30, 2012, 5:42 p.m.) > > > Review request for KMymoney. > > > Description > ------- > > 1. please note that dependency on Boost is no longer optional (see changes in > CMakeLists.txt) > > 2. Architectural changes > Until now, the method MyMoneyStatementReader::processTransactionEntry() > handled the whole process of importing a transaction - that is: handling the > securities, matching and creating payees, and - at the very end of the method > - adding the transaction to the ledger. > The last step (adding transaction to ledger) is the main target of this > refactoring. Its algorithm was as follows: > 1. find a matching transaction (either existing or scheduled) - using > TransactionMatcher::findMatch() > 2. If an "existing transaction match" is found - handle it (in the old > code it's the block starting with a comment "// it matched a simple > transaction. that's the easy case") > 3. Else if a "scheduled transaction match" is found - handle it ("// a > match has been found in a pending schedule"...) > > Code "mapping" is as follows: > - step 2 (above) is extracted to handleMatchingOfExistingTransaction() > - step 3 (above) is extracted to handleMatchingOfScheduledTransaction() > - TransactionMatcher::findMatch() is extracted to > TransactionMatchFinder::findMatch() (note: there are two pure-virtual > functions that are implemented in ExistingTransactionMatchFinder, > ScheduledTransactionMatchFinder classes) > - TransactionMatcher::checkTransaction() is extracted to > TransactionMatchFinder::findMatchingSplit() > > 3. Memory management changes > Raw pointers are no longer used, as these are typically error-prone. > Pointers were replaced either with object instances, or boost::optional is > used if applicable (e.g. see members of TransactionMatchFinder class). > > 4. dynamic_casts removed (were used on pointers returned by > TransactionMatcher::findMatch(), no longer needed - see > TransactionMatchFinder::getMatchedTransaction() and getMatchedSchedule() ) > > 5. variable/method names - I did my best to keep those meaningful: e.g. > "importedTransaction" instead of "t") > > > Diffs > ----- > > kmymoney/dialogs/transactionmatchfinder.cpp PRE-CREATION > kmymoney/dialogs/transactionmatcher.cpp c380877 > kmymoney/dialogs/transactionmatchfinder.h PRE-CREATION > CMakeLists.txt 93af070 > kmymoney/converter/mymoneystatementreader.h 758ff00 > kmymoney/converter/mymoneystatementreader.cpp 42c4841 > kmymoney/dialogs/CMakeLists.txt 9a8d782 > kmymoney/dialogs/existingtransactionmatchfinder.h PRE-CREATION > kmymoney/dialogs/existingtransactionmatchfinder.cpp PRE-CREATION > kmymoney/dialogs/scheduledtransactionmatchfinder.h PRE-CREATION > kmymoney/dialogs/scheduledtransactionmatchfinder.cpp PRE-CREATION > kmymoney/dialogs/transactionmatcher.h d09a4cd > > Diff: http://git.reviewboard.kde.org/r/107137/diff/ > > > Testing > ------- > > make test > > > Thanks, > > Łukasz Maszczyński > >
_______________________________________________ KMyMoney-devel mailing list KMyMoney-devel@kde.org https://mail.kde.org/mailman/listinfo/kmymoney-devel
