sdementen / piecash

Pythonic interface to GnuCash SQL documents
Other
290 stars 73 forks source link

The importance of Decimal? #206

Closed tim-rohrer closed 1 year ago

tim-rohrer commented 1 year ago

Working on a migration project, I'm using a mapper class to prepare my data for ingestion by Split.

@dataclass
class Split2Move:
    """Class to keep track of splits to be moved."""

    account: str
    value: str
    memo: str

In doing so, I realized that I did not explicitly use Decimal on the string value, and I subsequently mapped the data into Split. The code appears to work.

In looking at Split code in the docs, I see self.value = value, but nothing that converted it to decimal:Decimal.

Should my code have worked? Is this an accident (bug) waiting to happen?

I realize I can (and probably should) change my dataclass to enforce the attribute type, but I wanted to try and understand the situation better.

Thanks.

sdementen commented 1 year ago

Are you using the sqlite backend? If so, the sqlite backend is quite flexible in what it accepts. Do you get back Decimals?

tim-rohrer commented 1 year ago

Yes, sqlite.

    for tr in book.transactions:
        print(type(tr.splits[0].value))

<class 'decimal.Decimal'>

Appears so.

Sounds like I don't need to worry about then.

I have another issue, but will open anew if you prefer?

I'm using create_book as a memory server for testing (pytest) and I thought initially the problem was that book wasn't getting cleaned up and recreated totally new. I thought this because if I only run the second test, it works. If I run both, the second fails. I've seen this occur with jest and nearly always it was due to a mock not getting reset properly.

My two tests: https://bpa.st/BPYY6

With book.close() present, I get a session error If I remove the book.close() from the first test, I get

sqlalchemy.orm.exc.DetachedInstanceError: Parent instance <Account at 0x11292f5d0> is not bound to a Session; lazy load operation of attribute 'commodity' cannot proceed (Background on this error at: http://sqlalche.me/e/13/bhk3)

../../../.local/share/virtualenvs/move2gnucash-cDrekBd3/lib/python3.11/site-packages/sqlalchemy/orm/strategies.py:717: DetachedInstanceError

...when I make the call to create_accounts in the second test.

When I remove the book.close() from the first test, I get:

               KeyError: "Could not find object with {'fullname': Account<[USD]>} in []"

../../../.local/share/virtualenvs/move2gnucash-cDrekBd3/lib/python3.11/site-packages/piecash/_common.py:180: KeyError

...in my production code.

This seems to imply the two instances of book are not independent, no?

And I don't understand why.

The function I'm testing:

def add_transactions(book: Book, transactions_list):

    def build_split(split_params: Split2Move):
        """Builds Split entries for transaction being added to book."""
        split_params.account = book.accounts(fullname=split_params.account)
        return Split(**split_params.__dict__)

    for tr in transactions_list:
        tr.splits = [build_split(split) for split in tr.splits]
        tr.currency = book.commodities(mnemonic=tr.currency)
        Transaction(**tr.__dict__)
        book.flush()
    book.save()
sdementen commented 1 year ago

Looking at https://stackoverflow.com/questions/58649529/how-to-create-multiple-memory-databases-in-sqlite3, it seems that you need to use named in memory databases to avoid all these to point to the same database (and having cryptic error messages)

tim-rohrer commented 1 year ago

Turns out the test issue was related to my inexperience with python variables, and how this pattern:

for f in f_list:
  f.element = some_other_value

actually mutates f_list.

:sheepish_grin:

Thanks again for your help.