Creating abstract transaction_item class?

[topherbuckley]

In the abstract_transaction.init there are a bunch of checks for whether some of what I call "transaction_items" (e.g. unique_id, timestamp, etc.) are empty or unknown. While writing some updates for PR 134 on the manual csv input plugin, I'm finding it error prone to try to make sure all the things I pass to InTransaction, OutTransaction, and IntraTransaction meet the requirements set by the abstract_transaction.__init__ requirements.

Ideally I'd think we could have something similar to the Keywords class that can serve as a global reference for what the properties of each of these "transaction_items" should be. So for example, what if instead of returning a str here

@property
    def unique_id(self) -> str:
        return self.__unique_id

we returned a Unique_ID that inherits from Transaction_Item that looked something like:

class Transaction_Item:
    def __init__(
        self,
        disallow_empty: bool,
        disallow_unknown: bool,
        transaction_item_type: Transaction_Item_Type # e.g. Transaction_Item_Type.UNIQUE_ID ...
    ) -> None:
        self.__disallow_empty: bool = disallow_empty
        self.__disallow_unknown: bool = disallow_unknown
        self.__transaction_item_type: Transaction_Item_Type = transaction_item_type # or potentially transaction_item_type.value and type be str, but I think thats just less robust

    @property
    def disallow_empty(self) -> bool:
        return self.__disallow_empty

    @property disallow_unknown -> bool:
        return self.__disallow_unknown
    
    @property transaction_item_type -> Transaction_Item_Type:
        return self.__transaction_item_type

Then somewhere else we could define an Enum like Keywords that defines all the possible transaction_item_types in Transaction_Item_Type.

In this way, in all the input plugins, like the manual csv one, I can ensure all the input parameters to the InTransaction, OutTransaction, and IntraTransaction conform to the required values of empty and unknown without having to reference abstract_transaction.py when writing value checkers like these. I.e. right now, some of them don't get filled with the correct values (e.g. some are filled with __unknown when they shouldn't be). Rather than essentially rewrite the checks of abstract_transaction this would allow me to set the value of all optional transaction_items with the correct value of __unknown or empty.

I think this would reduce the amount of duplicate checks going on like here as well as reduce the amount of work developers would have to do to ensure future plugins provide the correct input parameters to abstract_transaction.__init__.

One last thing to note, is the need of this becomes a lot more apparent if you checkout my PR 134 where I'm trying to generalize things into Dicts rather than have a instance variable name for every transaction_item_type as was previously done. This reduces the amount of duplicate code by quite a bit.

[topherbuckley]

Added some code to support this idea in PR 177. I may still have some linting errors, but the idea is there.

[eprbell]

In the abstract_transaction.init there are a bunch of checks for whether some of what I call "transaction_items" (e.g. unique_id, timestamp, etc.) are empty or unknown. While writing some updates for PR 134 on the manual csv input plugin, I'm finding it error prone to try to make sure all the things I pass to InTransaction, OutTransaction, and IntraTransaction meet the requirements set by the abstract_transaction.__init__ requirements.

Ideally I'd think we could have something similar to the Keywords class that can serve as a global reference for what the properties of each of these "transaction_items" should be. So for example, what if instead of returning a str here

@property
    def unique_id(self) -> str:
        return self.__unique_id

we returned a Unique_ID that inherits from Transaction_Item that looked something like:

These properties are used to simulate direct read-only access to the value of a given field, so I don't think we want to change its signature: these values are used all over the place in expressions and functions and they have an intuitive use. Returning a class instance instead of a value would have major side effects on the entire code base.

However I'm still trying to understand the proposal. What is the advantage of capturing type constraints in a class vs a function call?

[topherbuckley]

These properties are used to simulate direct read-only access to the value of a given field, so I don't think we want to change its signature: these values are used all over the place in expressions and functions and they have an intuitive use. Returning a class instance instead of a value would have major side effects on the entire code base.

Roger that. That wasn't the major motivation for this, just something I thought might be useful. I've got a different place I can gather all of this and still be useful.

However I'm still trying to understand the proposal. What is the advantage of capturing type constraints in a class vs a function call?

I think it will be more apparent after I finish this PR so can I defer your question until then? I'll leave comments in the PR diff to exemplify it.