WonYong-Jang commented on PR #70622:
URL: https://github.com/apache/airflow/pull/70622#issuecomment-5150269160
> 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
Thank you for the review! Applied all three changes.
--
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]
