Conversation
potiuk
left a comment
There was a problem hiding this comment.
LGTM. Verified this is genuinely redundant rather than a load-bearing no-op: BaseTrigger._set_context is itself pass ("Part of LoggingMixin and used mainly for configuration of task logging; not used for triggers"), so the override duplicates its own base exactly and removal changes nothing.
Also checked the fallback on older cores — both LoggingMixin._set_context implementations are no-ops too, so there is no Airflow version where dropping this override starts doing something — and that SnowflakeSqlApiTrigger was the last trigger in the tree still carrying it, so it now matches the rest.
Nit: the PR description is just the unfilled template. A one-liner saying "BaseTrigger already defines _set_context as a no-op, so this override is redundant" would have saved a reviewer the trip through LoggingMixin — and it becomes the commit body on merge.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Airflow maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.More on how Apache Airflow handles maintainer review:
contributing-docs/05_pull_requests.rst.
Thanks for the review. I think your agent missed the PR description I wrote. I mentioned the following: "The |
9b015a8 to
3c134be
Compare
This PR removes the redundant
_set_contextin theSnowflakeSqlApiTrigger. TheBaseTrigger._set_contextis already a no-op pass, so there is no need for it in this trigger.Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.