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

smart_importer with bean-extract patch will post to first posting account despite 'filing_account' #97

Closed
scanta2 opened this issue Apr 4, 2024 · 14 comments

Comments

@scanta2
Copy link

scanta2 commented Apr 4, 2024

Filing_account has no effect for either banking or investment importers.
Using smart_importer together with the patched bean-extract leads to file names related to the first posting in each transaction entry, which is not necessarily the asset account.

@scanta2
Copy link
Author

scanta2 commented Apr 4, 2024

In addition, investment accounts get their file name from the ticker account, and not from the parent account.

@redstreet
Copy link
Owner

In addition, investment accounts get their file name from the ticker account, and not from the parent account.

Probably unrelated, would you mind opening a separate bug for this?

@redstreet
Copy link
Owner

From here.

See this comment. This line should be made the default for all transaction builders.

@scanta2
Copy link
Author

scanta2 commented Apr 4, 2024

@redstreet That's the temporary change I tried in my local repo, for banking and investment accounts. Would it be better to change the parent class instead?

@redstreet
Copy link
Owner

Correct. Patch coming in a few minutes.

@redstreet
Copy link
Owner

Does that fix work?

@awtimmering
Copy link
Contributor

I stepped through the smart_importer code, and it seems to be just a fluke of the prediction (maybe in situations where input data is not very rich yet). I verified the training data does not contain any reversed postings and it outputs it nonethless as result of predictor.py#L234.

We could consider proposing a change to update_postings(transaction, accounts) (entries.py#L6) to honor the original order (i.e. the single posting in transaction to always come in the first place, other accounts to follow), wdyt?

@redstreet
Copy link
Owner

Good to know nothing in particular caused this. Looks like we (reds-importers) were depending on the ordering so far, that we shouldn't have.

In general, ordering of postings in the Beancount ecosystem is not guaranteed AFAIK, so we shouldn't depend on that, or ask others to, as it would be prone to breakage.

I also like the idea of printing out a sorted order (even though it makes no functional difference), which we can only do if we have a different way of marking the file to write the transaction to.

The above patch I committed yesterday should resolve the problem in a dependable way. Have you tried it?

@awtimmering
Copy link
Contributor

In general, ordering of postings in the Beancount ecosystem is not guaranteed AFAIK, so we shouldn't depend on that, or ask others to, as it would be prone to breakage.

Agree. I think regardless of the fix on your side (which is good) it might be a good suggestion for smart_importers, so will suggest & add PR. There might be other considerations, but the current result ends up with postings that are hard to read, even if functionally not different, as the "remainder" entry can come in first place.

For example, an import for a bank transaction might end up like this:

2024-04-04 * "Bank payment A"
  Expenses:Groceries
  Assets:MyBank:Savings         -100 USD

The above patch I committed yesterday should resolve the problem in a dependable way. Have you tried it?

Sorry to say I haven't, as for a mix of reasons I am not using your importers (yet, anyway) - even though I have drawn a lot of inspiration from your posts and scripts! (My Japanese bank CSVs don't have headers, then my Japanese Amex CSV does have headers but has multiple sections for family cards that require some custom parsing etc).

@scanta2
Copy link
Author

scanta2 commented Apr 5, 2024

Good to know nothing in particular caused this. Looks like we (reds-importers) were depending on the ordering so far, that we shouldn't have.

In general, ordering of postings in the Beancount ecosystem is not guaranteed AFAIK, so we shouldn't depend on that, or ask others to, as it would be prone to breakage.

I also like the idea of printing out a sorted order (even though it makes no functional difference), which we can only do if we have a different way of marking the file to write the transaction to.

The above patch I committed yesterday should resolve the problem in a dependable way. Have you tried it?

Thank you for the fix, it works well on my side, and it also fixes #98.

@redstreet
Copy link
Owner

Thank you for the fix, it works well on my side, and it also fixes #98.

Great! Thanks for reporting.

@redstreet
Copy link
Owner

Agree. I think regardless of the fix on your side (which is good) it might be a good suggestion for smart_importers, so will suggest & add PR. There might be other considerations, but the current result ends up with postings that are hard to read, even if functionally not different, as the "remainder" entry can come in first place.

+1. I too find a lot of value in friendly posting ordering. After all, a core benefit of plain text accounting is the journal being human readable. Thanks for filing this with smart_importer.

@redstreet
Copy link
Owner

Sorry to say I haven't, as for a mix of reasons I am not using your importers (yet, anyway) - even though I have drawn a lot of inspiration from your posts and scripts! (My Japanese bank CSVs don't have headers, then my Japanese Amex CSV does have headers but has multiple sections for family cards that require some custom parsing etc).

Glad to hear the articles helped inspire! The process of writing those articles actually helped me get my 5-minute update down to just a few seconds, reliably. So it's well worth it IMHO.

Regardless of whether you use my importers: if you don't have headers, petl makes it trivial to inject them. Takes just a line of code.

Does your Amex contain multiple tables? If so, check out the multitable reader in my importers, either for use or for inspiration. Also based on petl.

@awtimmering
Copy link
Contributor

Added issue & PR at beancount/smart_importer#128 .

Thank you for the comments - I've been looking at petl and am keen to try it out, so will give it a try, as well as take another look at your importers. (And haven't started yet really undertanding the zsh scripts -- never touched that shell so far but seems I should give it a try!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants