disallow_empty, disallow_unknown, and None values

[topherbuckley]

Can someone help me understand the intent behind disallow_empty? I thought I understood, that it would check if if some value had len(value)==0 as per here but I'm quite confused by things like

    def _validate_optional_string_field(cls, name: str, value: Optional[str], raw_data: str, disallow_empty: bool, disallow_unknown: bool) -> Optional[str]:
        if not value:
            return None
        return cls._validate_string_field(name, value, raw_data, disallow_empty, disallow_unknown)

from here

It would seem the check for whether something was len(value)==0 will never be checked because the above statement will return None instead of error out when value==None. I would have thought None is also "empty", but it seems this is interpreted differently here.

The main reason I ask is because I was trying to interpret the "required" and "optional" parameters to each of the In/Out/Intra transactions listed in the config readme with regards to these disallow_empty and disallow_unknown. Generally speaking it seems that disallow_unknown appears to be of no consequence to being "required" or "optional", whereas anything that is disallow_empty would be required, and anything that is allowed to be empty, is optional. The only outlier there is the fiat_ticker which appears to be "optional" due to the fact that this value is filled in with the native fiat in a few places in the transaction_resolver

if transaction.fiat_ticker not in {None, global_configuration[Keyword.NATIVE_FIAT.value]}:

This seems very opaque to me, since here

        self.__fiat_ticker: Optional[str] = self._validate_optional_string_field(
            "fiat_ticker", fiat_ticker, raw_data, disallow_empty=True, disallow_unknown=True
        )

It is disallow_empty, so I would think that a None value would fail. These validate methods appear to let these None values through, and some are handled by transaction_resolver.

So in the end, I am planning to just add my own field specifying is_required in some AbstractTransactionItem class to help clear these things up, but if there is a way to determine if something is required or not just by the values of disallow_empty and disallow_unknown please let me know.

Looking at it all again, I guess the easiest way for me to determine if something is required or optional is to just look at the method call where it is verified (e.g. _validate_optional_string_field vs _validate_string_field). Is there a reason it was implemented this way? In other words, I'm curious if it doesn't make more things more clear to have a third bool called required in all of the constructors for things extending AbstractTransaction, and then just have one unified validate method for strings. I guess you lose some type hinting for the return value then. Anyways, I think I can add some clarity on this after getting my AbstractTransactionItem class finished.

[eprbell]

Regarding _validate_optional_string_field I think what you found a bug. The proper check should be:

if value is None:
   ...

Thanks for digging through this code and finding bugs.

As for the other question on optional vs required, the easy way to tell if a parameter is optional or required is just to look at the signature of the constructor: if a parameter has Optional in the type hint it's optional, otherwise it's required.

However I think I am missing some context: can you elaborate on why you need optional/required information in your PR and how you plan to use it?

[topherbuckley]

@macanudo527 just tagging you into this as I know you worked on the manual csv input plugin, so curious if you have any rational you can share with the above comment.

[macanudo527]

@topherbuckley Sorry, I wish I could help you out, but I didn't work on manual.py this is part of the core that @eprbell developed.

[eprbell]

@macanudo527 just tagging you into this as I know you worked on the manual csv input plugin, so curious if you have any rational you can share with the above comment.

The code you pasted handles intra transactions in the manual plugin. Such transactions can be "partial", which is why holder and exchange fields can be left blank: take a look at the relevant section of the manual plugin documentation, which explains the semantics. For example you may have a transaction from Coinbase (which is supported by RP2) to an exchange that is not supported. In this case you let the Coinbase plugin handle the send side of the transaction and then you add the receiving half of the transaction in the manual csv file. The manual csv transaction would have only to_exchange and to_holder populated (from_exchange and from_holder are left empty and they are assigned __unknown by the code). Hope this helps.

[topherbuckley]

@eprbell thanks! Yes, I'm sure that will set me off in the right direction and gives me context with which to logically figure out why things are one way over another. I'll come back to this a few days, but will remind myself to look down the stack to see how these from_exchange and from_holder are error checked later on (i.e. why __unknown would be more appropriate than None or Empty). I think this should help clarify a few things.

[topherbuckley]

@topherbuckley Sorry, I wish I could help you out, but I didn't work on manual.py this is part of the core that @eprbell developed.

Whoops, sorry, my mistake. Not sure where I came up with that.