For discussion: consider removing ApiAbstract

[mesozoic]

This is a proposal for discussion. It's not a "must have". I think it's worth considering prior to a major version release, since 2.0.0 is our best opportunity to make potentially breaking API changes.

tl;dr

I think we should consider removing ApiAbstract. I think the library API would be simpler for implementers to understand if it favored composition over inheritance. I think this will make it easier to add more endpoints in the future.

Why change it?

I'm suggesting this change to the structure of the API because:

1. It involves a lot of duplication. Adding one method (like Ignore extra fields in ORM (fixes #190, option 1) #250) involves adding three other methods which call the abstract class via super(). We need to keep the signatures and API documentation for each of those methods up-to-date.

2. This will not scale gracefully if we want to add dozens of endpoints. Keeping four separate method signatures up-to-date for every action we might take (update a webhook, post a comment, etc) will become burdensome over time.

3. The current approach exposes methods which don't make sense. Take the following code snippet as an example:

   >>> table = Table("key", "app", "tbl")
   >>> table.get_table_url()
   Traceback (most recent call last):
     File "<stdin>", line 1, in <module>
   TypeError: ApiAbstract.get_table_url() missing 2 required positional arguments: 'base_id' and 'table_name'

   ...the correct thing to do is call table.table_url rather than table.get_table_url(), but the presence of both (a byproduct of inheritance) is confusing clutter, especially if you're using an IDE that does autocompletion.

4. There are a number of endpoints (particularly around metadata) that only ever make sense in the context of a base, or a table, or a record; keeping those methods scoped to just the one appropriate class will simplify maintenance and make it easier for implementers to understand their use.

How would this work?

This is just a proposal, but one way I might arrange this is:

• ApiAbstract goes away
• Api contains basic utilities for connecting to the Airtable API, and nothing more.
  • __init__(access_token)
  • build_url(...) -> str
  • build_request(...) -> requests.Request
  • prepare_request(...) -> requests.PreparedRequest
  • request(...) -> dict executes a request and returns the deserialized JSON payload.
  • base(base_id) -> Base (renamed from get_base)
  • table(base_id, table_name) -> Table (renamed from get_table)
  • create_base (to be implemented)
• Base is where we'd add future methods for interacting with schemas, webhooks, etc.
  • __init__(api, base_id) could take an instance of Api or str
  • api, url would be available as properties
  • table(table_name) -> Table (renamed from get_table)
  • create_table (to be implemented)
• Table would contain a lot of the methods that exist in ApiAbstract today.
  • __init__(access_token: str, base_id: str, table_name: str) (old style constructor)
  • __init__(base: Base, table_name: str) (new style constructor)
  • api, base, url would be available as properties
  • record_url(record_id) -> str (renamed from get_record_url)
  • get(record_id: str, **options)
  • iterate(**options)
  • first(**options)
  • all(**options)
  • create(fields: dict)
  • batch_create(records)
  • update(record_id: str, fields: dict)
  • batch_update(records: List[dict])
  • batch_upsert(records: List[dict], key_fields: List[str])
  • delete(record_id: str)
  • batch_delete(record_ids: List[str])
  • update_schema (to be implemented)
  • create_field (to be implemented)
  • update_field (to be implemented)

So the most typical use cases would still be supported:

>>> table = Table("token", "appWhatever", "tblWhatever")
>>> records = table.all(formula=formula)
>>> # do stuff

...but could also be achieved through composition, which might be useful in some circumstances (depending on how the implementer is using this library):

>>> api = Api("token")
>>> table = Table(api, "appWhatever", "tblWhatever")
>>> records = table.all(formula=formula)

...or perhaps like this:

>>> api = Api("token")
>>> table = api.base("appWhatever").table("tblWhatever")
>>> records = table.all(formula=formula)

There would be less confusion over inherited methods that don't quite apply:

>>> table.get_table_url()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'Table' object has no attribute 'get_table_url'
>>> table.url
'https://api.airtable.com/v0/appWhatever/tblWhatever'

...and there would be less boilerplate required every time we need to add a method, since we'd only ever add it in one place.

Why shouldn't we do this?

I can think of two big reasons:

1. Breaking API changes are never fun and maybe we just don't want the hassle. I think this can be mitigated by (A) getting the constructors to take old-style arguments (strs) as well as new-style arguments (instances), and (B) using wrappers that emit deprecation warnings to preserve some of the old method names for a period of time.

2. It's not strictly necessary. We could start adding lots of things to the existing Base or Table classes and not add them to the ApiAbstract base class. We'd be breaking an existing pattern, though, and it would be somewhat arbitrary which methods existed in all places and which ones only existed where they "belong". There would also still be some methods in ApiAbstract which don't belong in subclasses (like get_table_url).

I'm open to hearing about more reasons not to consider this 😁

[gtalarico]

@mesozoic I think that's pretty compelling and a great approach.
I personally be ok with a clean break on the major bump. I think the majority of users tend to use only the table class, but your call .