potiuk commented on PR #70831:
URL: https://github.com/apache/airflow/pull/70831#issuecomment-5149226791
This gets the split right, which is worth saying explicitly because several
PRs in this series have not.
The "both provided" case is a provision check — passing both `statement` and
`statement_id` is a Dag-authoring error whatever they render to — so `__init__`
with `is not None` polarity is the correct home, and that polarity also catches
`statement_id=""`, which a truthiness check would let through.
Leaving the "neither resolves" case in `execute()` is right too, and I
checked rather than assumed: `template_fields` includes both `statement` and
`statement_id`, so `statement="{{ params.sql }}"` rendering to an empty string
is only visible after rendering. `__init__` genuinely cannot catch that one.
The asymmetry is correct, not an oversight.
Tests are split to match, which reads well.
One thing before merge. The relocated line still raises `AirflowException`:
```python
if statement is not None and statement_id is not None:
raise AirflowException("Cannot provide both statement and statement_id.")
```
Moving one verbatim during a refactor is explicitly allowed, and the file's
ratchet entry stays at `::4`, so nothing is violated as it stands. But the
guidance is to narrow when you're already touching the line, and this is an
argument-combination error, so `ValueError` fits — exactly what you did in
#70634:
```python
if statement is not None and statement_id is not None:
raise ValueError("Cannot provide both statement and statement_id.")
```
That needs the test's `pytest.raises(AirflowException, ...)` updated to
`ValueError`, and `generated/known_airflow_exceptions.txt` dropped from
`providers/databricks/src/airflow/providers/databricks/sensors/databricks.py::4`
to `::3`.
Happy to merge once that's in.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
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]
