rjgoyln commented on code in PR #72703:
URL: https://github.com/apache/airflow/pull/72703#discussion_r3960338602
##########
shared/timezones/src/airflow_shared/timezones/timezone.py:
##########
@@ -231,13 +231,20 @@ def td_format(td_object: None | dt.timedelta | float |
int) -> str | None:
For example timedelta(seconds=3752) would become `1h:2M:32s`.
If the time is less than a second, the return will be `<1s`.
+ A negative duration is formatted by magnitude and prefixed with `-`,
+ so timedelta(seconds=-3752) would become `-1h:2M:32s`.
Review Comment:
The sign rule has one exception that the docstring doesn't mention: a
magnitude below one second still renders as an unsigned `<1s`, so
`td_format(-0.4) == td_format(0.4) == "<1s"`. That's a reasonable choice — and
the PR description explains it — but a reader of the docstring alone would
expect a `-` here.
```suggestion
A negative duration is formatted by magnitude and prefixed with `-`,
so timedelta(seconds=-3752) would become `-1h:2M:32s`. A magnitude below
one second is still `<1s`, without a sign.
```
##########
shared/timezones/src/airflow_shared/timezones/timezone.py:
##########
@@ -231,13 +231,20 @@ def td_format(td_object: None | dt.timedelta | float |
int) -> str | None:
For example timedelta(seconds=3752) would become `1h:2M:32s`.
If the time is less than a second, the return will be `<1s`.
+ A negative duration is formatted by magnitude and prefixed with `-`,
+ so timedelta(seconds=-3752) would become `-1h:2M:32s`.
"""
if td_object is None:
return None
+ # Format the magnitude and re-apply the sign at the end. Formatting a
negative
+ # duration directly does not work: the day-to-month division below floors,
so
+ # e.g. days=-1 becomes months=-1, days=+29, and `_format_part` then drops
the
+ # negative month and leaves the 29 days behind.
+ is_negative = td_object < dt.timedelta(0) if isinstance(td_object,
dt.timedelta) else td_object < 0
if isinstance(td_object, dt.timedelta):
- delta = relativedelta() + td_object
+ delta = relativedelta() + abs(td_object)
else:
- delta = relativedelta(seconds=int(td_object))
+ delta = relativedelta(seconds=int(abs(td_object)))
Review Comment:
The ternary re-runs the `isinstance` check that the very next `if` performs.
Folding the sign decision into the existing branch keeps one type dispatch
instead of two:
```suggestion
if isinstance(td_object, dt.timedelta):
is_negative = td_object < dt.timedelta(0)
delta = relativedelta() + abs(td_object)
else:
is_negative = td_object < 0
delta = relativedelta(seconds=int(abs(td_object)))
```
--
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]
