gtalarico / pyairtable

Python Api Client for Airtable
https://pyairtable.readthedocs.io
MIT License
786 stars 139 forks source link

ORM CheckboxField returns None for false values #193

Closed NicoHood closed 1 year ago

NicoHood commented 2 years ago

Hi, I ran into the issue, that a checkbox field returns None even though it should return false. I guess that is because airtable will leave out the result in the api call and then the api just interprets that as None. I guess the reason for that is that in the code it is marked to return Optional[bool].

Not sure yet how to fix that.

NicoHood commented 2 years ago

Maybe this is a solution, but I dont know how hacky this is. to me it makes more sense to actually throw the exception on an empty value and handle this exception in the parent class. But it might not make any difference at all.

class CheckboxField(Field):
    """Airtable Checkbox field. Uses ``bool`` to store value"""

    def to_internal_value(self, value: Any) -> bool:
        if value is None:
            print('xxxx')
        print(value)
        return bool(value)

    def valid_or_raise(self, value) -> None:
        if not isinstance(value, bool):
            raise ValueError("CheckboxField value must be 'bool'")

    def __get__(self, *args, **kwargs) -> Optional[bool]:
        value = super().__get__(*args, **kwargs)
        if value is None:
            return False
        return value
NicoHood commented 2 years ago

I have the same issue for strings. We should handle how our fields should behave on null values. I personally did expect an empty string here as well.

larsakerson commented 2 years ago

This is a constant bother with the Airtable API: empty cells return a null response rather than an empty string, etc. I wouldn't be opposed to having the model return empty strings for str types, empty lists for list types, etc. Are there downsides to consider?

gtalarico commented 2 years ago

IMHO empty string col should return "" but empty bool col should return None

be opposed to having the model return empty strings for str types, empty lists for list types, etc. Are there downsides to consider?

That seems wise to me

I guess the reason for that is that in the code it is marked to return Optional[bool]. I think bool field should be able to store None. We can update class to allow that and not try to cast None to False