Re: [PR] Cleanup: Enforce keyword-only session in db_cleanup [airflow]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #63781:
URL: https://github.com/apache/airflow/pull/63781#discussion_r3025337004


##########
airflow-core/src/airflow/utils/db_cleanup.py:
##########
@@ -652,7 +653,10 @@ def export_archived_records(
 
 @provide_session
 def drop_archived_tables(
-    table_names: list[str] | None, needs_confirm: bool, session: Session = NEW_SESSION
+    *,
+    table_names: list[str] | None,
+    needs_confirm: bool,

Review Comment:
   `drop_archived_tables` is now fully keyword-only (`*, table_names, needs_confirm, session`), which is a larger breaking change than “keyword-only session”. If you only need `session` to be keyword-only (per Airflow core guideline), you can keep `table_names`/`needs_confirm` positional-or-keyword and introduce `*, session` instead.
   ```suggestion
       table_names: list[str] | None,
       needs_confirm: bool,
       *,
   ```



##########
airflow-core/tests/unit/utils/test_db_cleanup.py:
##########
@@ -741,6 +741,16 @@ def test_drop_archived_tables(self, mock_input, confirm_mock, inspect_mock, capl
         else:
             confirm_mock.assert_not_called()
 
+    def test_export_archived_records_positional_fails(self):
+        with pytest.raises(TypeError) as exc:
+            export_archived_records("csv", "path", ["table"], True, True, None)
+        assert "takes 2 positional arguments but 6" in str(exc.value)
+
+    def test_drop_archived_tables_positional_fails(self):
+        with pytest.raises(TypeError) as exc:
+            drop_archived_tables(["table"], True)
+        assert "takes 0 positional arguments but 2" in str(exc.value)

Review Comment:
   These tests assert on a specific CPython `TypeError` wording/argument count substring (e.g. "takes 2 positional arguments but 6"). The exact message format can vary across Python versions/implementations, making this brittle. Consider asserting only that a `TypeError` is raised, or use `pytest.raises(..., match=...)` with a looser regex that doesn’t depend on the exact count/message text.
   ```suggestion
           with pytest.raises(TypeError, match="positional arguments"):
               export_archived_records("csv", "path", ["table"], True, True, None)
   
       def test_drop_archived_tables_positional_fails(self):
           with pytest.raises(TypeError, match="positional arguments"):
               drop_archived_tables(["table"], True)
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org