Key Account.transactions by account type instead of str

[teeberg]

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]

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]

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]

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]

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

[isaacharrisholt]

Couple of things: next *minor release

Secondly, could we format with a hanging indent? :)

            warnings.warn(
                f'You passed in a value of {header!r} for argument "header", '
                f'but it should have been of type "AccountType". Starting with '
                f'the next major release, this will become a type error.',
                DeprecationWarning,
           )

[isaacharrisholt]

Also, the second and third lines don't need to be f-strings

[teeberg]

Are you looking to follow semver? According to semver, a breaking change requires a major release, which is why I wrote this here. :) Happy to change the copy here, though, if you don't mind.

[teeberg]

and, changed it to a hanging indent

[isaacharrisholt]

Oh really? I thought a breaking change could have a minor release, and it was just hotfixes that had to be version compatible? Maybe I need to do some reading...

[isaacharrisholt]

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

[isaacharrisholt]

Reading this again, I'm perhaps a little confused. If we were always converting to an account type, could we not just change the type hint for the transactions field?

[teeberg]

We were not necessarily always converting since you can also call .set_header() manually outside of this function

[isaacharrisholt]

True, but surely we could just do self.header = AccountType(header) there? That should work fine, and throw and error if it's an invalid account type, which we want? I don't mind people being able to pass in strings (hence the strings everywhere in the docs and tests), as long as things end up the right types in the objects. Also, everything is pretty well type-hinted, so people's IDEs should scream at them if they try and use the wrong things in the wrong places.

At the end of the day, there's only so much defensive programming we can do. Python is inherently flexible, and you'll always be able to break things like this.

[teeberg]

I'm happy to change that, but that could potentially be a breaking change, if people used .set_header() to set other strings that are not valid QIF account types. Shall I?

[isaacharrisholt]

It's not a breaking change if it fixes unintended behaviour 🤷🏻‍♂️

I think we should probably fix this, then run a static type checker like pyright against the codebase. I'm also tempted to introduce black for formatting. That's what I've done with iTmpl and it makes life so much easier.

[teeberg]

Ok, fine with me!

[isaacharrisholt]

Let's just make sure to squash the commits on this PR before merging 😅

[teeberg]

sure, will do :D

[teeberg]

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]

Squashed everything. How does that look?

[teeberg]

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:

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

[isaacharrisholt]

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]

What are these comments for?

[teeberg]

It shuts up type warnings in the IDE:

Do you want me to take those out?

[teeberg]

I took them out :)

[isaacharrisholt]

Thanks. I think they're perhaps specific to your editor

[teeberg]

Yeah, I think they're IntelliJ-specific

[isaacharrisholt]

@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]

Do you have merge permissions?

[teeberg]

Do you have merge permissions?

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