ORM table `kwargs` wrapping

[BAPCon]

In regards to issues like #187 and #186 , are we looking to wrap records related fields in the ORM module? Not sure if those are issues we are actively looking to solve.

You can already achieve, for example, return_fields_by_field_id when invoking functions in the Model like cls.all() and cls.first(). However, that requires the user to repeat the kwargs on each call. Would an explicit approach that allows these options to be assigned in the Meta subclass fit into the overall roadmap?

@classmethod
    def _kwargs_union(cls, request_args: dict):
        meta_keys = [
            'view', 
            'fields', 
            'sort', 
            'formula', 
            'cell_format', 
            'user_locale', 
            'time_zone', 
            'return_fields_by_field_id'
        ]
        args_union = {attr: cls._get_meta(attr) for attr in meta_keys if cls._get_meta(attr)}
        args_union.update(request_args)
        return args_union

@classmethod
    def all(cls, **kwargs: Any) -> List[SelfType]:
        """
        Retrieve all records for this model. For all supported
        keyword arguments, see :meth:`Table.all <pyairtable.Table.all>`.
        """
        table = cls.get_table()
        kwargs = cls._kwargs_union(kwargs)
        return [cls.from_record(record) for record in table.all(**kwargs)]

This then allows:

class Contact(Model):
    name = fields.TextField("fldxqagV60dzo00fV")
    class Meta:
        base_id = "base"
        table_name = "table"
        api_key = "your_key"
        timeout = (5, 5)
        typecast = True
        return_fields_by_field_id=True

[mesozoic]

I can definitely see a rationale for putting Api kwargs into the model configuration, since otherwise there's no way to provide them.

For the table kwargs, I think it's worth going case by case:

• 👍 return_fields_by_field_id – I suppose if someone wants to set up the ORM to use field IDs instead of field names, this might be useful. It would need some testing to see if there are other side effects. Right now if someone passed this to Model.all() it would probably just break.
• 👍 typecast – I candidly can't see it making a lot of sense to turn this off, especially if you're type checking your code (which will prevent you from e.g. writing a str to a DatetimeField). But I also don't see a lot of harm in making this a model configuration option.
• 👎 fields – it does not make sense to pass this to Model.all(). If anything, the Model class should be passing this automatically based on how its own fields have been configured, so it doesn't get more data from the API than it actually needs.
• 👎 sort, page_size, max_records – I definitely don't think these belong in the model configuration since they're going to be so specific to a particular use case. They should stay as a kwarg for the all() method.
• ❓ view and formula – I'm not sure these belong in the model configuration, since you can still create records which don't match the view/formula conditions. So it could lead to a situation where you create a record through the model, call save(), and then can't retrieve or fetch it later.
• 👎 cell_format – using this makes the ORM sort of irrelevant, since everything will come back as a str. I could be convinced otherwise if someone comes in with a legit use case.
• 👎 time_zone and user_locale are only relevant if cell_format="string". IMHO these features aren't that useful; it should be up to the client application, not the Airtable server, to localize the data it receives from the API.

[BAPCon]

return_fields_by_field_id does not appear to break Model.all(), and I think it might be the only justifiable addition to the model configuration. I am not understanding what typecast does currently in the ORM code. Is it only on Model.save() that it checks types?

[mesozoic]

As I understand it, typecast=True means that Airtable will make best effort to convert a string value to the appropriate data type. That means you can submit "2024-03-18" and it will coerce to a date; you can submit a record name to a linked record field, and Airtable will attempt to find the existing record (or create a new one). I'm ambivalent on whether this should default to true or false, but I think making it a Meta option is sensible. It's really only meaningful if you're not using mypy to type-check your code, though.

I don't think return_fields_by_field_id works in the current codebase because the return values won't match any of the existing field descriptors. I tested this here:

>>> import os
>>> from pyairtable.orm import Model, fields as f
>>> class Contact(Model):
...   class Meta:
...     api_key = os.environ['AIRTABLE_API_KEY']
...     base_id = 'appaPqizdsNHDvlEm'
...     table_name = 'Contact'
...   name = f.TextField('First Name')
...
>>> print(Contact.all()[0].name)
Alice
>>> print(Contact.all(return_fields_by_field_id=True)[0].name)
None

So I think this only makes sense as a Meta configuration option, indicating the model is configured with field IDs instead of field names. I don't think we should allow passing the return_fields_by_field_id parameter to Model.all() at all.

These would both be welcome additions to the 3.0 release :)

[mesozoic]

On further inspection, it looks like we do already support timeout and typecast as model options (here and here). So what's left here, imho, is:

1. Support a use_field_ids option in Model.Meta that forces return_fields_by_field_id=True.
2. Disallow kwargs like cell_format or return_fields_by_field_id in Model.all()

[BAPCon]

Sounds good, I agree with everything said.