Modify PDFExtractor to support skippedItems (Part 1)#5533
Modify PDFExtractor to support skippedItems (Part 1)#5533gamue wants to merge 8 commits intoportfolio-performance:masterfrom
Conversation
...en/portfolio/datatransfer/pdf/raiffeisenbankgruppe/RaiffeisenbankgruppePDFExtractorTest.java
Outdated
Show resolved
Hide resolved
...sts/src/name/abuchen/portfolio/datatransfer/pdf/postfinance/PostfinancePDFExtractorTest.java
Outdated
Show resolved
Hide resolved
...src/name/abuchen/portfolio/datatransfer/pdf/traderepublic/TradeRepublicPDFExtractorTest.java
Outdated
Show resolved
Hide resolved
|
Hi @gamue, I propose not add more banks to this pull request. Instead, let's start a new pull request for the other banks. This change is not a simple "do a refactoring without semantic change". It needs more review, and then it is easier to do this on smaller diffs. Are the banks here in the pull request don from your point of view? Let me know when I should take a look at the pull request |
Agree, and actually had the same idea :) Was already reviewing it more in depth on my own. Think the current state should be fine for review, just a few notes/thoughts:
|
gamue
changed the title
| return null; | ||
| return item; | ||
|
|
||
| return new SkippedItem(item, Messages.MsgErrorTransactionTypeNotSupportedOrRequired); |
There was a problem hiding this comment.
@gamue
I am reviewing the changes to the PDF importer (and started with Scalable - one step at a time).
I am not sure about the side effects of this change.
The block pattern is a simple ^Erstattete Steuern[\\s]*$ and then there is only one matching, but optional element. There is no test that hits the skipped item.
You introduced this TaxLostAdjustmentTransaction. What was the reason to mark it optional() in the first place?
If we convert this to a SkippedItem, then - by definition - PP will stop attempting to try other blocks. My gut feeling is we should return a skipped item only if we reliably know that we parsed a transaction. But maybe you have example cases?
|
|
||
| if (t.getCurrencyCode() != null && t.getAmount() == 0) | ||
| ctx.markAsFailure(Messages.MsgErrorTransactionTypeNotSupportedOrRequired); | ||
| return new SkippedItem(item, Messages.MsgErrorTransactionTypeNotSupportedOrRequired); |
There was a problem hiding this comment.
This is exactly the kind of transactions we want to mark as skipped.
2 participants
Closes #5517
Especially in the AccountStatement-methods that work with very generic regexs like
"^[\\d]{2} [\\p{L}]{3,4}([\\.]{1})?.*$"it's not possible to change thereturn null, otherwise there will be lots ofSkippedItem. I think best to keep it as it. wdyt?The following ones will be handled in a follow-up PR: