potiuk commented on code in PR #65936:
URL: https://github.com/apache/airflow/pull/65936#discussion_r3148588501
##########
airflow-core/src/airflow/models/variable.py:
##########
@@ -46,6 +46,33 @@
log = logging.getLogger(__name__)
+def _build_upsert_stmt(
Review Comment:
nit: scope is a bit ambiguous. The helper is private (`_`-prefix) and
`Variable`-specific in practice but generically named, which is the worst of
both worlds. If reuse for `Connection` / `Pool` / etc. isn't on the immediate
roadmap, rename to `_build_variable_upsert_stmt` to match what the function
actually does. If reuse *is* the plan, move it into the same util module that
hosts `get_dialect_name` (with a `type[Base]` annotation) so the next model can
pull it in without a second refactor.
##########
airflow-core/src/airflow/models/variable.py:
##########
@@ -46,6 +46,33 @@
log = logging.getLogger(__name__)
+def _build_upsert_stmt(
Review Comment:
nit: tighten the type hints. As written, `model: type` accepts any class
(including non-SQLAlchemy ones, which would fail at runtime with a confusing
error), and `values: dict` / `update_fields: dict` are `dict[Any, Any]` which
discards the key/value typing. The original code had `stmt: MySQLInsert |
PostgreSQLInsert | SQLiteInsert` as a local annotation — lifting that to the
return position keeps mypy happy.
Suggested:
```python
def _build_upsert_stmt(
dialect: str | None,
model: type[Variable],
conflict_cols: list[str],
values: dict[str, Any],
update_fields: dict[str, Any],
) -> MySQLInsert | PostgreSQLInsert | SQLiteInsert:
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
