isaacharrisholt / quiffen

Quiffen is a Python package for parsing QIF (Quicken Interchange Format) files.
MIT License
34 stars 30 forks source link

Key `Account.transactions` by account type instead of `str` #49

Closed teeberg closed 1 year ago

teeberg commented 1 year ago

In account.py, the transactions field is defined as:

transactions: Dict[str, TransactionList] = {}

But there were a few places where we were using an instance of the AccountType enum as the key instead of the string, which could result in invalid QIF files being produced that read things like

!Type:AccountType.CASH

instead of

!Type:Cash
isaacharrisholt commented 1 year ago

I'm not sure what my preference would be here. I think I'd actually lean towards making the transactions field a Dict[AccountType, TransactionList] and altering the output methods to use the enum values, rather than the other way around.

What are your thoughts on this? The biggest downside is that it wouldn't be retro-compatible.

teeberg commented 1 year ago

I was thinking the exact same. I would also prefer making the transactions a Dict[AccountType, TransactionList], but since that's not backwards-compatible, I decided to pass strings instead. :)

I think changing it to AccountType could fairly easily be rolled out by releasing a new minor version that allows both and logs a warning if you pass in a str, to be backwards-compatible, and then either immediately or in a month or so release a new major version that only allows AccountType to be passed in.

isaacharrisholt commented 1 year ago

I agree, though I would use a new hotfix version for the initial one, and a minor version later. Would you be willing to make the initial changes in this PR?

teeberg commented 1 year ago

@isaacharrisholt Sure, did that. What do you think of the latest commit?

isaacharrisholt commented 1 year ago

Actually, thinking about this, I believe Pydantic will automatically convert the string to an enum if it's valid, will it not?

teeberg commented 1 year ago

Actually, thinking about this, I believe Pydantic will automatically convert the string to an enum if it's valid, will it not?

I briefly tested this and it doesn't seem to, as far as I can tell

teeberg commented 1 year ago

Squashed everything. How does that look?

teeberg commented 1 year ago

Actually, thinking about this, I believe Pydantic will automatically convert the string to an enum if it's valid, will it not?

Tested this some more. You're right it in that it automatically converts str to enum during object instantiation in a case like this:

image

It does not to do that at any later time, though, e.g. when re-setting any of the attributes.

isaacharrisholt commented 1 year ago

Tested this some more. You're right it in that it automatically converts str to enum during object instantiation in a case like this:

Yeah, I thought so. That's one of the benefits of Pydantic.

isaacharrisholt commented 1 year ago

@teeberg if you're interested, I've started work on the tooling improvements in #50

I'll wait for this one to be done first though, as it'll affect typing stuff.

isaacharrisholt commented 1 year ago

Do you have merge permissions?

teeberg commented 1 year ago

Do you have merge permissions?

I don't think so! Thanks for merging! :)