InputPlugin subclass' __init__ default parameters ignored

[topherbuckley]

Can someone explain the reasoning for the code below? (found here)

        if getattr(annotation, "__origin__", None) is Union and len(annotation.__args__) == 2 and annotation.__args__[1] is type(None):
            if ini_config[plugin_name].get(parameter) is not None:
                annotation = annotation.__args__[0]
            else:
                result[parameter] = None
                continue

It looks like it was introduced @ commit 66e194c with commit message:

complete blockfi support
    
    Added support for parsing the following transaction types in all
    transaction export CSV file:
    
    * Trade
    * Ach Deposit
    * Ach Withdrawal
    * Referral Bonus
    
    Also added support for parsing all trades CSV export and merge with all
    transactions exports.

and was apparently something to do with support for src/dali/plugin/input/csv/blockfi.py.

It is catching an Optional input parameters to the manual csv importer I'm working on (or any InputPlugin for that matter), and making the Parameter None. It appears setting them to None will result in ignoring the default values assigned it in the __init__ function of the manual csv importer's InputPlugin.__init__ (or any other subclass of InputPlugin). Not sure if this is intentionally or was otherwise unknown. Regardless it interferes with what I'm trying to do so I want to modify the logic without breaking whatever its original purpose was.

Can someone point me to what exactly in the src/dali/plugin/input/csv/blockfi.py used this logic to run successfully?

[eprbell]

I think that code added support for Optional parameters in plugin constructors (as you pointed out). Here's an example of use of an optional parameter in the blockfi plugin: https://github.com/eprbell/dali-rp2/blob/main/src/dali/plugin/input/csv/blockfi.py#L219.

Can you share the code you're writing? That way it's easier to comment on the behavior or on potential bugs. If you're trying to assign a default value to an optional parameter you could make the default be None and then inside the constructor check. E.g (assuming the parameter is an int called my_optional_par and you want it to have default value 7):

self.__my_optional_par = my_optional_par if my_optional_par is not None else 7

Hope this helps.

[topherbuckley]

Thanks for the speedy helpful reply!

Can you share the code you're writing?
Yeah, I'll update my PR with this after I figure out something that works and bring in some more specific comments. Feel free to ignore this whole discussion until I get that pushed. Still working out some silly typos in my commits. Hopefully tomorrow I can come back to this.

Is it necessary to set it to None in the first place though? I need to test this, but I was under the impression that if you just fail to supply the optional parameters, then the default __init__ assignments will actually take effect. If you specify them as None then the defaults will simply be ignored, because the user specified they wanted them to be None.

Here's an example of use of an optional parameter in the blockfi plugin: https://github.com/eprbell/dali-rp2/blob/main/src/dali/plugin/input/csv/blockfi.py#L219.
Ok, so long as I can preserve the value of self.__trade_csv_file is defined here the code in dali_main.py I pointed to can be modified?:

    def __init__(
        self,
        account_holder: str,
        transaction_csv_file: str,
        trade_csv_file: Optional[str] = None,
        native_fiat: Optional[str] = None,
    ) -> None:
        super().__init__(account_holder=account_holder, native_fiat=native_fiat)
        self.__transaction_csv_file: str = transaction_csv_file
        self.__trade_csv_file: Optional[str] = trade_csv_file
        self.__logger: logging.Logger = create_logger(null)

Are you aware of any existing test that would test the value of self.__trade_csv_file as None and something else?

[eprbell]

I don't think the BlockFi plugin has tests unfortunately. However you could probably test this by printing trade_csv_file and initializing the plugin manually with different values for trade_csv_file.