Repository: gtalarico/pyairtable (python)

Issue:
ORM get all() entries returns a json, not a Model
Talking about this:
https://github.com/gtalarico/pyairtable/blob/main/pyairtable/orm/model.py#L234-L238

This confuses me, as it really does not help for the ORM. I am currently trying to download existing data **as Model**. I thought that the `all()` function would do that, but it returns the json data, which is not what we want.

Pull request:
Return models, not dicts, from Model.{all,first}
Today these methods return dicts, and now they'll return model instances. This one was straightforward but I wrote a couple more tests and dried up a couple others.

Fixes #188.

Patch:
diff --git a/pyairtable/orm/model.py b/pyairtable/orm/model.py
index bbc5338..7d7c394 100644
--- a/pyairtable/orm/model.py
+++ b/pyairtable/orm/model.py
@@ -61,13 +61,12 @@ True
 
 """
 import abc
-from typing import List, Type, TypeVar
+from typing import List
 
-from pyairtable import Table
-
-from .fields import Field
+from typing_extensions import Self
 
-T = TypeVar("T", bound="Model")
+from pyairtable import Table
+from pyairtable.orm.fields import Field
 
 
 class Model(metaclass=abc.ABCMeta):
@@ -233,16 +232,22 @@ class Model(metaclass=abc.ABCMeta):
         return result["deleted"]
 
     @classmethod
-    def all(cls, **kwargs) -> List[T]:
-        """Returns all records for this model. See :meth:`~pyairtable.api.Api.all`"""
+    def all(cls, **kwargs) -> List[Self]:
+        """
+        Returns all records for this model. For the full list of
+        keyword arguments, see :meth:`~pyairtable.api.Api.all`
+        """
         table = cls.get_table()
-        return table.all(**kwargs)
+        return [cls.from_record(record) for record in table.all(**kwargs)]
 
     @classmethod
-    def first(cls, **kwargs) -> List[T]:
-        """Returns first record for this model. See :meth:`~pyairtable.api.Api.first`"""
+    def first(cls, **kwargs) -> Self:
+        """
+        Returns the first record for this model. For the full list of
+        keyword arguments, see :meth:`~pyairtable.api.Api.all`
+        """
         table = cls.get_table()
-        return table.first(**kwargs)
+        return cls.from_record(table.first(**kwargs))
 
     def to_record(self, only_writable: bool = False) -> dict:
         """
@@ -264,7 +269,7 @@ class Model(metaclass=abc.ABCMeta):
         return {"id": self.id, "createdTime": self.created_time, "fields": fields}
 
     @classmethod
-    def from_record(cls: Type[T], record: dict) -> T:
+    def from_record(cls, record: dict) -> Self:
         """Create instance from record dictionary"""
         name_field_map = cls._field_name_descriptor_map()
         # Convert Column Names into model field names
@@ -284,7 +289,7 @@ class Model(metaclass=abc.ABCMeta):
         return instance
 
     @classmethod
-    def from_id(cls: Type[T], record_id: str, fetch=True) -> T:
+    def from_id(cls, record_id: str, fetch=True) -> Self:
         """
         Create an instance from a `record_id`
 
diff --git a/setup.cfg b/setup.cfg
index ff4a672..5a74a88 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -30,6 +30,7 @@ classifiers =
 packages = find:
 install_requires =
     requests >= 2.22.0
+    typing_extensions
     urllib3 < 2
 
 [aliases]


Test patch:
diff --git a/tests/test_orm.py b/tests/test_orm.py
index 123883a..3fa1126 100644
--- a/tests/test_orm.py
+++ b/tests/test_orm.py
@@ -1,4 +1,5 @@
 from datetime import datetime
+from operator import itemgetter
 from unittest import mock
 
 import pytest
@@ -55,21 +56,24 @@ def test_model_overlapping():
             exists = f.TextField("Exists")  # clases with Model.exists()
 
 
-def test_model():
-    class Address(Model):
-        Meta = fake_meta(table_name="Address")
-        street = f.TextField("Street")
-        number = f.TextField("Number")
-
-    class Contact(Model):
-        Meta = fake_meta(table_name="Contact")
-        first_name = f.TextField("First Name")
-        last_name = f.TextField("Last Name")
-        email = f.EmailField("Email")
-        is_registered = f.CheckboxField("Registered")
-        link = f.LinkField("Link", Address, lazy=True)
-        birthday = f.DateField("Birthday")
+class Address(Model):
+    Meta = fake_meta(table_name="Address")
+    street = f.TextField("Street")
+    number = f.TextField("Number")
+
+
+class Contact(Model):
+    Meta = fake_meta(table_name="Contact")
+    first_name = f.TextField("First Name")
+    last_name = f.TextField("Last Name")
+    email = f.EmailField("Email")
+    is_registered = f.CheckboxField("Registered")
+    address = f.LinkField("Link", Address, lazy=True)
+    birthday = f.DateField("Birthday")
+    created_at = f.CreatedTimeField("Created At")
 
+
+def test_model():
     contact = Contact(
         first_name="Gui",
         last_name="Talarico",
@@ -104,24 +108,19 @@ def test_model():
 
 
 def test_from_record():
-    class Contact(Model):
-        Meta = fake_meta()
-        first_name = f.TextField("First Name")
-        timestamp = f.DatetimeField("Timestamp")
-
     # Fetch = True
     with mock.patch.object(Table, "get") as m_get:
         m_get.return_value = {
             "id": "recwnBLPIeQJoYVt4",
             "createdTime": "",
-            "fields": {"First Name": "X", "Timestamp": "2014-09-05T12:34:56.000Z"},
+            "fields": {"First Name": "X", "Created At": "2014-09-05T12:34:56.000Z"},
         }
         contact = Contact.from_id("recwnBLPIeQJoYVt4")
 
     assert m_get.called
     assert contact.id == "recwnBLPIeQJoYVt4"
     assert contact.first_name == "X"
-    assert contact.timestamp.year == 2014
+    assert contact.created_at.year == 2014
 
     # Fetch = False
     with mock.patch.object(Table, "get") as m_get_no_fetch:
@@ -136,11 +135,6 @@ def test_readonly_field_not_saved():
     but we can retrieve readonly fields and set them on instantiation.
     """
 
-    class Contact(Model):
-        Meta = fake_meta(table_name="Contact")
-        birthday = f.DateField("Birthday")
-        age = f.IntegerField("Age", readonly=True)
-
     record = {
         "id": "recwnBLPIeQJoYVt4",
         "createdTime": datetime.utcnow().isoformat(),
@@ -163,14 +157,6 @@ def test_readonly_field_not_saved():
 
 
 def test_linked_record():
-    class Address(Model):
-        Meta = fake_meta(table_name="Address")
-        street = f.TextField("Street")
-
-    class Contact(Model):
-        Meta = fake_meta(table_name="Contact")
-        address = f.LinkField("Link", Address, lazy=True)
-
     record = {"id": "recFake", "createdTime": "", "fields": {"Street": "A"}}
     address = Address.from_id("recFake", fetch=False)
 
@@ -187,36 +173,41 @@ def test_linked_record():
     assert contact.address[0].street == "A"
 
 
-def test_undeclared_field__from_id(requests_mock):
+@pytest.mark.parametrize(
+    "test_case",
+    [
+        ("from_id", lambda cls, id: cls.from_id(id)),
+        ("first", lambda cls, _: cls.first()),
+        ("all", lambda cls, _: cls.all()[0]),
+    ],
+    ids=itemgetter(0),
+)
+def test_undeclared_field(requests_mock, test_case):
     """
-    Test that Model.from_id ignores any fields which are missing from the Model definition.
+    Test that Model methods which fetch data from the Airtable API will
+    ignore any fields which are missing from the Model definition.
+
     See https://github.com/gtalarico/pyairtable/issues/190
     """
 
-    class JustName(Model):
-        Meta = fake_meta()
-        name = f.TextField("Name")
+    record = fake_record(
+        Number="123",
+        Street="Fake St",
+        City="Springfield",
+        State="IL",
+    )
 
-    record = fake_record({"Name": "Alice", "Address": "123 Fake St"})
     requests_mock.get(
-        JustName.get_table().get_record_url(record["id"]),
+        Address.get_table().table_url,
+        status_code=200,
+        json={"records": [record]},
+    )
+    requests_mock.get(
+        Address.get_table().get_record_url(record["id"]),
         status_code=200,
         json=record,
     )
 
-    instance = JustName.from_id(record["id"])
-    assert instance.to_record()["fields"] == {"Name": "Alice"}
-
-
-def test_undeclared_field__all():
-    """
-    Test that Model.all ignores any fields which are missing from the Model definition.
-    """
-    pytest.skip("To be implemented in 2.0.0; see #249")  # TODO
-
-
-def test_undeclared_field__first():
-    """
-    Test that Model.first ignores any fields which are missing from the Model definition.
-    """
-    pytest.skip("To be implemented in 2.0.0; see #249")  # TODO
+    _, get_model_instance = test_case
+    instance = get_model_instance(Address, record["id"])
+    assert instance.to_record()["fields"] == {"Number": "123", "Street": "Fake St"}