nanjeshramesh commented on PR #70576:
URL: https://github.com/apache/airflow/pull/70576#issuecomment-5158589994
Thanks for the thorough review!
**On the `except ValueError` catch-all** — good catch, that was a real gap.
A genuinely invalid static `target_time` (e.g. `"not-a-date"`) was being
treated the same as an unrendered Jinja template and silently deferred instead
of failing at Dag-parse time like before. Fixed by checking for actual Jinja
delimiters (`{{`/`{%`) before assuming "unrendered template" — anything else
re-raises immediately. Added
`test_async_start_from_trigger_invalid_static_target_time_fails_fast` to cover
it.
**On `_resolve_moment`'s validation check** — `__init__` does guarantee one
of `moment`/`target_time` is set, so this is only a defensive guard against
`target_time` being cleared after construction (rather than surfacing a
confusing `AttributeError` later). Added a comment explaining that instead of
removing it.
**Docstrings/comments** — trimmed all of them to roughly your suggested
wording (the `start_from_trigger` param doc, the template-hand-off comment, the
pre-3.3 fallback comment, the `target_time` param doc on `DateTimeTrigger`, and
the `serialize()` comment).
Pushed as three commits: fail-fast fix + trimmed docs, further comment
trims, and the new test.
--
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]
