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

Balance wrong for csvbase importers with multiple transactions in a day #145

Open
rbrunt opened this issue Jan 4, 2025 · 1 comment
Open

Comments

@rbrunt
Copy link

rbrunt commented Jan 4, 2025

I've just migrated over to beancount3, and while doing so I decided to move my importers over to use the new csvbase.Importer base class since the older csv base class has now been marked as deprecated.

Overall, this has been a great change - it's now much simpler to understand how to implement them, and they're much smaller now, so easier to understand what's going on.

The only issue I've encountered is that the automatic balance assertions always fail when there are multiple transactions on the last day of the csv's listings, since it takes the first entry in the last day. All of the csv files I have from my bank are in ascending date order, so this means that the balance is always off by the amount of the last transaction.

I've got a new (failing) test that reproduces the issue to communicate what it is that I mean here: rbrunt@528deba

    @docfile
    def test_extract_balance_with_multiple_transactions_per_day_and_ascending_dates(self, filename):
        """\
        2021-05-17, Test A, 1.00, 1.00
        2021-05-18, Test B, 2.00, 3.00
        2021-05-18, Test C, 1.00, 4.00
        """
        class CSVImporter(Base):
            date = Date(0)
            narration = Column(1)
            amount = Amount(2)
            balance = Amount(3)
            names = False
        importer = CSVImporter('Assets:CSV', 'EUR')
        entries = importer.extract(filename, [])
        self.assertEqualEntries(entries, """
        2021-05-17 * "Test A"
          Assets:CSV  1.00 EUR
        2021-05-18 * "Test B"
          Assets:CSV  2.00 EUR
        2021-05-18 * "Test C"
          Assets:CSV  1.00 EUR
        2021-05-19 balance Assets:CSV  4.00 EUR
        """)

I'm happy to write a tweak to csvbase.Importer to implement this behaviour how I would have expected it to work (to get this new test to pass), but before I do, I wanted to check if there were any views on approach: I don't want to change behaviour here and affect others who update to a new version because the base class has changed under them, but I also think this is likely to be catching quite a few people out.

The easiest approach would be to move from using max to a more explicit pick:

for currency, balances in balances.items():
            ascending = balances[0].date >= balances[-1].date
            last_balance = balances[0] if ascending else balances[-1]
            entries.append(last_balance)

However this might break existing importers people have already written, so perhaps pulling this bit of code out into a separate function that can be overriden by a subclass or mixin would be a safer bet.

@blais
Copy link
Member

blais commented Jan 6, 2025

SGTM, this seems like a bug. Please send a patch.

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

2 participants