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

PDF import for Computershare #3732

Closed
wants to merge 1 commit into from

Conversation

jimjaeger
Copy link

Pullrequest contains a pdf extractor and testcase for computershare documents .
Unfortunately CS only provides one document type and only purchase entries.
Dividends are only shown on the page but not in documents.

@Nirus2000
Copy link
Member

Nirus2000 commented Jan 14, 2024

Thank you for the pull request.
However, I think that this differs considerably from all the standards of the many other PDF importers and thus occupies a special position. We want to avoid this and have therefore developed a standard.

Please have a look at the other importers or the example in the contributing rules.

Regards
Alex

// guessing
final Pattern singleDatePattern = Pattern.compile(PATTERN_STRING_SINGLE_DATE);
final Pattern doubleDatePattern = Pattern.compile(PATTERN_STRING_DOUBLE_DATE);
final Pattern cusipPattern = Pattern.compile(PATTERN_STRING_CUSIP);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the thinking for extracting the pattern/attributes names into constants.
For reasons of readability, we have decided against it for most of the time.
Take the cusip pattern: the constant is defined and then used once, but now it is defined far away (in terms of lines) so that one cannot glance to see the context.

The thinking was: attributes must be consistent between match and assign but not beyond. So we scope it those methods.

Other than that it looks good to me (for now only looked in the browser)

@Nirus2000 Nirus2000 added the pdf label Jan 18, 2024
t.setAmount(asAmount(v.get(PATTERN_KEY_AMOUNT)));
t.setShares(asShares(v.get(PATTERN_KEY_SHARES)));
final Security security = getOrCreateSecurity(v);
security.setCurrencyCode(asCurrencyCode("USD"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not allowed.
The security matched by the importer might already exist in the data file of the user.
Therefore we must not change the currency (also considering that the security already has transactions and historical prices attached that assume another currency).

I will merge the importer but will change this before.

buchen added a commit that referenced this pull request Jan 25, 2024
Issue: #3732
Co-authored-by: Andreas Buchen <[email protected]>
@buchen
Copy link
Member

buchen commented Jan 25, 2024

Merged. With changes.

@buchen buchen closed this Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants