gtalarico / pyairtable

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

Drop `ApiAbstract` #267

Closed mesozoic closed 1 year ago

mesozoic commented 1 year ago

This closes #257 by removing ApiAbstract. Instead, we have Api, Base, and Table classes that each have separate functions and responsibilities. They do still refer to each other, and their constructors have at least some backwards compatibility. See below (or read the changes to migrations.rst) for more details.

This will be a breaking API change, so definitely want to get eyes on this before I merge it. Tagging recent contributors for ideas or feedback. @BAPCon @goksan @larsakerson @NicoHood @marks

A note on circular dependencies

Api needs to know how to create Base and Table instances, but the Base and Table instances need to have references back to the Api and Base (respectively). To avoid a circular reference, these modules import each other using fully qualified names, and only refer to the classes within methods that need them. Type annotations are enclosed in quotes.

This results in internal code that might be a bit aesthetically displeasing, like:

class Base:
    def table(self, table_name):
        return pyairtable.api.table.Table(table_name)

class Table:
    api: "pyairtable.api.api.Api"
    base: "pyairtable.api.base.Base"

This was, however, the simplest form I could find that allows for both type annotations and bidirectional runtime dependencies. Api needs to be able to create a Base, but for backwards compatibility, we also need Base to be able to create an Api.

A note on backwards compatibility

The most common use case for this library (based on a cursory search) seems to be:

table = Table(api_key, base_id, table_name)

This branch preserves backwards compatibility with that constructor syntax, but will emit deprecation warnings implying that this style could be removed in the future:

>>> from pyairtable import Base, Table
>>> base = Base("api_key", "base_id")
<stdin>:1: DeprecationWarning: Passing API keys to pyairtable.Base is deprecated; use Api.base() instead. See https://pyairtable.rtfd.org/en/latest/migrations.html for details.
>>> table = Table("api_key", "base_id", "table_name")
<stdin>:1: DeprecationWarning: Passing API keys or base IDs to pyairtable.Table is deprecated; use Api.table() or Base.table() instead. See https://pyairtable.rtfd.org/en/latest/migrations.html for details.

The rationale for deprecating this code path is that, in the future, we might want to perform additional steps when referencing a table (such as validating its existence). This will be easier to do in Base.table() before constructing the Table instance.

Summary of each commit

codecov[bot] commented 1 year ago

Codecov Report

Merging #267 (98a336a) into main (6e95f05) will increase coverage by 1.26%. The diff coverage is 95.60%.

:exclamation: Current head 98a336a differs from pull request most recent head e5485bc. Consider uploading reports for the commit e5485bc to get more accurate results

@@            Coverage Diff             @@
##             main     #267      +/-   ##
==========================================
+ Coverage   93.30%   94.56%   +1.26%     
==========================================
  Files          16       15       -1     
  Lines         836      755      -81     
==========================================
- Hits          780      714      -66     
+ Misses         56       41      -15     
Impacted Files Coverage Δ
pyairtable/api/retrying.py 100.00% <ø> (ø)
pyairtable/api/types.py 100.00% <ø> (ø)
pyairtable/formulas.py 86.00% <ø> (ø)
pyairtable/metadata.py 26.31% <0.00%> (-3.10%) :arrow_down:
pyairtable/utils.py 100.00% <ø> (ø)
pyairtable/api/base.py 95.45% <95.45%> (+17.95%) :arrow_up:
pyairtable/api/table.py 98.09% <97.87%> (-1.91%) :arrow_down:
pyairtable/api/api.py 100.00% <100.00%> (+22.50%) :arrow_up:
pyairtable/orm/model.py 96.00% <100.00%> (+0.34%) :arrow_up:
mesozoic commented 1 year ago

I'm working on some other branches to finish up the rest of the 2.0 release scope, but they depend on this one so I'm not posting them yet. I'll plan to merge this branch next week if there are no objections or questions raised here.