potiuk commented on PR #70622:
URL: https://github.com/apache/airflow/pull/70622#issuecomment-5148897977
Good catch — this is a real bug. `AirflowException("...%s...", value)`
doesn't interpolate; both arguments are kept, so the message rendered as a
tuple rather than a path. The f-string is the right fix, and tightening the
test to an exact-equality assertion against the real path is exactly what it
needed.
One thing before this goes in. The project is actively reducing direct
`AirflowException` usages, and the guidance is that when you touch a line that
raises one, narrow it rather than leave it behind. Here the `except` clause
tells you the specific failure — the path isn't there — so `FileNotFoundError`
fits:
```python
except NoSuchPathError as e:
# Protection should the bare repo be removed manually
raise FileNotFoundError(f"Repository path: {self.bare_repo_path} not
found") from e
```
The same method already does this a few lines above, so it stays consistent
with its neighbours:
```python
raise RuntimeError("Error cloning repository") from e
raise RuntimeError(f"Invalid git repository at {self.bare_repo_path}") from e
```
Two follow-on edits if you take it:
- the test's `pytest.raises(AirflowException)` becomes
`pytest.raises(FileNotFoundError)`
- `generated/known_airflow_exceptions.txt` has
`providers/git/src/airflow/providers/git/bundles/git.py::5` — drop it to `::4`,
otherwise the ratchet check fails
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]
