Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parsing a part of a PDF requires a start and an end line #630

Merged
merged 4 commits into from Oct 22, 2016

Conversation

@akoch
Copy link
Contributor

@akoch akoch commented Oct 7, 2016

Otherwise the parser cannot handle PDF files containing more than one
transaction, e.g. OnVista

akoch added 2 commits Oct 7, 2016
Otherwise the parser cannot handle PDF files containing more than one
transaction, e.g. OnVista
Improved regex for block recognition of SBroker
Recognizing deposit transactions works now with multipart parsing
Fixed SBroker unit test
@buchen
Copy link
Owner

@buchen buchen commented Oct 8, 2016

Hi @akoch - vielen Dank für den Patch!

Mir ist bewußt, dass der PDFParser#Section potentiell mehr Zeilen sehen kann (sprich: es könnte eine Überlauf in den nächsten Block geben). Genauso sieht jede PDFParser#Section nochmal alle Zeilen.

Ich denke das kann man so ändern. Hast Du ein Beispiel wo sich das auswirkt? Wenn ich nämlich alle Deine Changes abzüglich der PDFParser Changes laufen lasse, dann sind alle Tests weiterhin grün. Die PDFParser Änderung scheint da auf den ersten Blick keinen Unterschied zu machen. Allerdings wieß ich nicht ob z.B. bei dem Matching des Wertpapiernamens im Onvista Exporter ein Problem auftreten könnte. Vielleicht gibt es ja dazu einen Testfall und ein assertThat Statement?

Andreas.

@akoch
Copy link
Contributor Author

@akoch akoch commented Oct 8, 2016

Hi @buchen ,

Zwei Dinge sind dazugekommen:
1.) Ein Unit-Test, der das Problem veranschaulicht. Ohne den obigen Patch wird die Orderprovision beiden Transaktionen zugerechnet, d.h. 5€ zuviel an erfassten Gebühren.
2.) Die Handelsplatzgebühr wird bei Onvista jetzt auch erfasst

Allerdings weiß ich nicht ob z.B. bei dem Matching des Wertpapiernamens im Onvista Exporter ein Problem auftreten könnte. Vielleicht gibt es ja dazu einen Testfall und ein assertThat Statement?

Die Anmerkung kann ich nicht genau zuordnen. Um welchen Export geht es?

Gruß,
Andreas

@buchen
Copy link
Owner

@buchen buchen commented Oct 10, 2016

Allerdings weiß ich nicht ob z.B. bei dem Matching des Wertpapiernamens im Onvista Exporter ein Problem auftreten könnte. Vielleicht gibt es ja dazu einen Testfall und ein assertThat Statement?

Die Anmerkung kann ich nicht genau zuordnen. Um welchen Export geht es?

Ich bezog mich damit auf die Änderung an dem OnvistaPDFExtractor:

 +                           type.getCurrentContext().put("nameP4", v.get("nameP4"));
 +                        })
 +
 +                        .section("combine")
 +                        .match("(?<combine>.*)")
 +                        .assign((t, v) -> {

Ich wollte sagen, ich habe keine Testfall gefunden, bei dem es einen Unterschied macht ob man "nameP4" als separate (optionale) Section definiert. Ist aber eigentlich auch egal - die Änderung sieht logisch aus.

Ich komme heute Abend nicht mehr dazu mir die Änderung im Detail anzuschauen und zu mergen. Werde ich im Laufe der Woche machen. Deine andere Änderung ist drin! 😄

@akoch
Copy link
Contributor Author

@akoch akoch commented Oct 11, 2016

Ohne die Änderung schlägt name.abuchen.portfolio.datatransfer.pdf.OnvistaPDFExtractorTest.testDepotauszug() fehl, weil er nicht mehr für jeden Depoteintrag in den Block mit "nameP4" läuft. Kann sein, dass man das eleganter lösen könnte, aber das erschien mir die naheliegendste Herangehensweise.

Wenn du noch verbesserungswürdige Stellen findest, sag Bescheid.

@buchen buchen merged commit 61e42bb into buchen:master Oct 22, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
buchen added a commit that referenced this pull request Oct 22, 2016
@akoch akoch deleted the akoch:PDF-Parser-Multipart branch Aug 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants