Repository: gtalarico/pyairtable (python)

Issue:
There should be a way to always set `return_fields_by_field_id` to `True` on all supported calls.
Is this already supported? If so, I have not been able to find it. If not, then it would be a welcome feature addition!

Pull request description:
Add `use_field_ids=` param to Api
This allows implementers to declare `use_field_ids=True` once, when creating the Api instance, and not have to pass it to each method call thereafter. FYI @jonathanlaniado 

Fixes #365 

Patch:
diff --git a/docs/source/_substitutions.rst b/docs/source/_substitutions.rst
index b5e230c..290da7d 100644
--- a/docs/source/_substitutions.rst
+++ b/docs/source/_substitutions.rst
@@ -58,7 +58,8 @@
     bet set to null. If ``False``, only provided fields are updated.
 
 .. |kwarg_use_field_ids| replace:: An optional boolean value that lets you return field objects where the
-    key is the field id. This defaults to `false`, which returns field objects where the key is the field name.
+    key is the field id. This defaults to ``False``, which returns field objects where the key is the field name.
+    This behavior can be overridden by passing ``use_field_ids=True`` to :class:`~pyairtable.Api`.
 
 .. |kwarg_force_metadata| replace::
     By default, this method will only fetch information from the API if it has not been cached.
diff --git a/pyairtable/api/api.py b/pyairtable/api/api.py
index 3f55281..a6fa6ea 100644
--- a/pyairtable/api/api.py
+++ b/pyairtable/api/api.py
@@ -41,6 +41,10 @@ class Api:
     # Cached metadata to reduce API calls
     _bases: Optional[Dict[str, "pyairtable.api.base.Base"]] = None
 
+    endpoint_url: str
+    session: Session
+    use_field_ids: bool
+
     def __init__(
         self,
         api_key: str,
@@ -48,6 +52,7 @@ class Api:
         timeout: Optional[TimeoutTuple] = None,
         retry_strategy: Optional[Union[bool, retrying.Retry]] = True,
         endpoint_url: str = "https://api.airtable.com",
+        use_field_ids: bool = False,
     ):
         """
         Args:
@@ -63,6 +68,8 @@ class Api:
                 (see :func:`~pyairtable.retry_strategy` for details).
             endpoint_url: The API endpoint to use. Override this if you are using
                 a debugging or caching proxy.
+            use_field_ids: If ``True``, all API requests will return responses
+                with field IDs instead of field names.
         """
         if retry_strategy is True:
             retry_strategy = retrying.retry_strategy()
@@ -74,6 +81,7 @@ class Api:
         self.endpoint_url = endpoint_url
         self.timeout = timeout
         self.api_key = api_key
+        self.use_field_ids = use_field_ids
 
     @property
     def api_key(self) -> str:
diff --git a/pyairtable/api/table.py b/pyairtable/api/table.py
index d885156..a9d4294 100644
--- a/pyairtable/api/table.py
+++ b/pyairtable/api/table.py
@@ -197,6 +197,8 @@ class Table:
             user_locale: |kwarg_user_locale|
             use_field_ids: |kwarg_use_field_ids|
         """
+        if self.api.use_field_ids:
+            options.setdefault("use_field_ids", self.api.use_field_ids)
         record = self.api.get(self.record_url(record_id), options=options)
         return assert_typed_dict(RecordDict, record)
 
@@ -230,6 +232,8 @@ class Table:
         """
         if isinstance(formula := options.get("formula"), Formula):
             options["formula"] = to_formula_str(formula)
+        if self.api.use_field_ids:
+            options.setdefault("use_field_ids", self.api.use_field_ids)
         for page in self.api.iterate_requests(
             method="get",
             url=self.url,
@@ -290,7 +294,7 @@ class Table:
         self,
         fields: WritableFields,
         typecast: bool = False,
-        use_field_ids: bool = False,
+        use_field_ids: Optional[bool] = None,
     ) -> RecordDict:
         """
         Create a new record
@@ -304,6 +308,8 @@ class Table:
             typecast: |kwarg_typecast|
             use_field_ids: |kwarg_use_field_ids|
         """
+        if use_field_ids is None:
+            use_field_ids = self.api.use_field_ids
         created = self.api.post(
             url=self.url,
             json={
@@ -318,7 +324,7 @@ class Table:
         self,
         records: Iterable[WritableFields],
         typecast: bool = False,
-        use_field_ids: bool = False,
+        use_field_ids: Optional[bool] = None,
     ) -> List[RecordDict]:
         """
         Create a number of new records in batches.
@@ -343,6 +349,8 @@ class Table:
             use_field_ids: |kwarg_use_field_ids|
         """
         inserted_records = []
+        if use_field_ids is None:
+            use_field_ids = self.api.use_field_ids
 
         # If we got an iterator, exhaust it and collect it into a list.
         records = list(records)
@@ -367,7 +375,7 @@ class Table:
         fields: WritableFields,
         replace: bool = False,
         typecast: bool = False,
-        use_field_ids: bool = False,
+        use_field_ids: Optional[bool] = None,
     ) -> RecordDict:
         """
         Update a particular record ID with the given fields.
@@ -384,6 +392,8 @@ class Table:
             typecast: |kwarg_typecast|
             use_field_ids: |kwarg_use_field_ids|
         """
+        if use_field_ids is None:
+            use_field_ids = self.api.use_field_ids
         method = "put" if replace else "patch"
         updated = self.api.request(
             method=method,
@@ -401,7 +411,7 @@ class Table:
         records: Iterable[UpdateRecordDict],
         replace: bool = False,
         typecast: bool = False,
-        use_field_ids: bool = False,
+        use_field_ids: Optional[bool] = None,
     ) -> List[RecordDict]:
         """
         Update several records in batches.
@@ -417,6 +427,8 @@ class Table:
         """
         updated_records = []
         method = "put" if replace else "patch"
+        if use_field_ids is None:
+            use_field_ids = self.api.use_field_ids
 
         # If we got an iterator, exhaust it and collect it into a list.
         records = list(records)
@@ -442,7 +454,7 @@ class Table:
         key_fields: List[FieldName],
         replace: bool = False,
         typecast: bool = False,
-        use_field_ids: bool = False,
+        use_field_ids: Optional[bool] = None,
     ) -> UpsertResultDict:
         """
         Update or create records in batches, either using ``id`` (if given) or using a set of
@@ -462,6 +474,9 @@ class Table:
         Returns:
             Lists of created/updated record IDs, along with the list of all records affected.
         """
+        if use_field_ids is None:
+            use_field_ids = self.api.use_field_ids
+
         # If we got an iterator, exhaust it and collect it into a list.
         records = list(records)
 
diff --git a/pyairtable/formulas.py b/pyairtable/formulas.py
index 6eb71df..5a7865d 100644
--- a/pyairtable/formulas.py
+++ b/pyairtable/formulas.py
@@ -466,7 +466,7 @@ def quoted(value: str) -> str:
     return "'{}'".format(value)
 
 
-def escape_quotes(value: str) -> str:
+def escape_quotes(value: str) -> str:  # pragma: no cover
     r"""
     Ensure any quotes are escaped. Already escaped quotes are ignored.
 


Test patch:
diff --git a/tests/test_api_table.py b/tests/test_api_table.py
index d432676..34beedc 100644
--- a/tests/test_api_table.py
+++ b/tests/test_api_table.py
@@ -1,3 +1,4 @@
+from datetime import datetime, timezone
 from posixpath import join as urljoin
 from unittest import mock
 
@@ -11,6 +12,8 @@ from pyairtable.models.schema import TableSchema
 from pyairtable.testing import fake_id, fake_record
 from pyairtable.utils import chunked
 
+NOW = datetime.now(timezone.utc).isoformat()
+
 
 @pytest.fixture()
 def table_schema(sample_json, api, base) -> TableSchema:
@@ -206,31 +209,62 @@ def test_first_none(table: Table, mock_response_single):
         assert rv is None
 
 
-def test_all(table: Table, mock_response_list, mock_records):
-    with Mocker() as mock:
-        mock.get(
-            table.url,
+def test_all(table, requests_mock, mock_response_list, mock_records):
+    requests_mock.get(
+        table.url,
+        status_code=200,
+        json=mock_response_list[0],
+        complete_qs=True,
+    )
+    for n, resp in enumerate(mock_response_list, 1):
+        offset = resp.get("offset", None)
+        if not offset:
+            continue
+        offset_url = table.url + "?offset={}".format(offset)
+        requests_mock.get(
+            offset_url,
             status_code=200,
-            json=mock_response_list[0],
+            json=mock_response_list[1],
             complete_qs=True,
         )
-        for n, resp in enumerate(mock_response_list, 1):
-            offset = resp.get("offset", None)
-            if not offset:
-                continue
-            offset_url = table.url + "?offset={}".format(offset)
-            mock.get(
-                offset_url,
-                status_code=200,
-                json=mock_response_list[1],
-                complete_qs=True,
-            )
-        response = table.all()
+    response = table.all()
 
     for n, resp in enumerate(response):
         assert dict_equals(resp, mock_records[n])
 
 
+@pytest.mark.parametrize(
+    "kwargs,expected",
+    [
+        ({"view": "Grid view"}, {"view": ["Grid view"]}),
+        ({"page_size": 10}, {"pageSize": ["10"]}),
+        ({"max_records": 10}, {"maxRecords": ["10"]}),
+        ({"fields": ["Name", "Email"]}, {"fields[]": ["Name", "Email"]}),
+        ({"formula": "{Status}='Active'"}, {"filterByFormula": ["{Status}='Active'"]}),
+        ({"cell_format": "json"}, {"cellFormat": ["json"]}),
+        ({"user_locale": "en_US"}, {"userLocale": ["en_US"]}),
+        ({"time_zone": "America/New_York"}, {"timeZone": ["America/New_York"]}),
+        ({"use_field_ids": True}, {"returnFieldsByFieldId": ["1"]}),
+        (
+            {"sort": ["Name", "-Email"]},
+            {
+                "sort[0][direction]": ["asc"],
+                "sort[0][field]": ["Name"],
+                "sort[1][direction]": ["desc"],
+                "sort[1][field]": ["Email"],
+            },
+        ),
+    ],
+)
+def test_all__params(table, requests_mock, kwargs, expected):
+    """
+    Test that parameters to all() get translated to query string correctly.
+    """
+    m = requests_mock.get(table.url, status_code=200, json={"records": []})
+    table.all(**kwargs)
+    assert m.last_request.qs == expected
+
+
 def test_iterate(table: Table, mock_response_list, mock_records):
     with Mocker() as mock:
         mock.get(
@@ -460,6 +494,113 @@ def test_delete_view(table, mock_schema, requests_mock):
     assert m.call_count == 1
 
 
+fake_upsert = {"updatedRecords": [], "createdRecords": [], "records": []}
+
+
+def test_use_field_ids__get_record(table, monkeypatch, requests_mock):
+    """
+    Test that setting api.use_field_ids=True will change the default behavior
+    (but not the explicit behavior) of Table.get()
+    """
+    record = fake_record()
+    url = table.record_url(record_id := record["id"])
+    m = requests_mock.register_uri("GET", url, json=record)
+
+    # by default, we don't pass the param at all
+    table.get(record_id)
+    assert m.called
+    assert "returnFieldsByFieldId" not in m.last_request.qs
+
+    # if use_field_ids=True, we should pass the param...
+    monkeypatch.setattr(table.api, "use_field_ids", True)
+    m.reset()
+    table.get(record_id)
+    assert m.called
+    assert m.last_request.qs["returnFieldsByFieldId"] == ["1"]
+
+    # ...but we can override it
+    m.reset()
+    table.get(record_id, use_field_ids=False)
+    assert m.called
+    assert m.last_request.qs["returnFieldsByFieldId"] == ["0"]
+
+
+@pytest.mark.parametrize("method_name", ("all", "first"))
+def test_use_field_ids__get_records(table, monkeypatch, requests_mock, method_name):
+    """
+    Test that setting api.use_field_ids=True will change the default behavior
+    (but not the explicit behavior) of Table.all() and Table.first()
+    """
+    m = requests_mock.register_uri("GET", table.url, json={"records": []})
+
+    # by default, we don't pass the param at all
+    method = getattr(table, method_name)
+    method()
+    assert m.called
+    assert "returnFieldsByFieldId" not in m.last_request.qs
+
+    # if use_field_ids=True, we should pass the param...
+    monkeypatch.setattr(table.api, "use_field_ids", True)
+    m.reset()
+    method()
+    assert m.called
+    assert m.last_request.qs["returnFieldsByFieldId"] == ["1"]
+
+    # ...but we can override it
+    m.reset()
+    method(use_field_ids=False)
+    assert m.called
+    assert m.last_request.qs["returnFieldsByFieldId"] == ["0"]
+
+
+@pytest.mark.parametrize(
+    "method_name,method_args,http_method,suffix,response",
+    [
+        ("create", ({"fields": {}}), "POST", "", fake_record()),
+        ("update", ("rec123", {}), "PATCH", "rec123", fake_record()),
+        ("batch_create", ([fake_record()],), "POST", "", {"records": []}),
+        ("batch_update", ([fake_record()],), "PATCH", "", {"records": []}),
+        ("batch_upsert", ([fake_record()], ["Key"]), "PATCH", "", fake_upsert),
+    ],
+)
+def test_use_field_ids__post(
+    table,
+    monkeypatch,
+    requests_mock,
+    method_name,
+    method_args,
+    http_method,
+    suffix,
+    response,
+):
+    """
+    Test that setting api.use_field_ids=True will change the default behavior
+    (but not the explicit behavior) of the create/update API methods on Table.
+    """
+    url = f"{table.url}/{suffix}".rstrip("/")
+    print(f"{url=}")
+    m = requests_mock.register_uri(http_method, url, json=response)
+
+    # by default, the param is False
+    method = getattr(table, method_name)
+    method(*method_args)
+    assert m.call_count == 1
+    assert m.last_request.json()["returnFieldsByFieldId"] is False
+
+    # if use_field_ids=True, we should pass the param...
+    monkeypatch.setattr(table.api, "use_field_ids", True)
+    m.reset()
+    method(*method_args)
+    assert m.call_count == 1
+    assert m.last_request.json()["returnFieldsByFieldId"] is True
+
+    # ...but we can override it
+    m.reset()
+    method(*method_args, use_field_ids=False)
+    assert m.call_count == 1
+    assert m.last_request.json()["returnFieldsByFieldId"] is False
+
+
 # Helpers