Repository navigation
Conversation
1AbdullahButt
left a comment
There was a problem hiding this comment.
The sign fix itself is right, and both Duration.in_words() and Interval.in_words() are covered. A rounding side effect is worth handling now that the sign isn't stripped, though. The value goes through :.2f, so anything between -0.005 s and 0 renders as negative zero:
>>> f"{-1000 / 1e6:.2f}", f"{-4999 / 1e6:.2f}"
('-0.00', '-0.00')So pendulum.duration(microseconds=-1000).in_words() would become "-0.00 seconds". Before this PR it was "0.00 seconds", which is also odd, but at least not signed. Options: round first and drop the sign when the rounded value is zero, e.g.
value = round(self.microseconds / 1e6, 2)
count = f"{value + 0.0:.2f}" # +0.0 turns -0.0 into 0.0or fall through to the microsecond unit when abs(microseconds) < 5000. A test with microseconds=-1000 would pin whichever you choose.
Separately, and pre-existing: Duration uses plural(0) ("seconds") while Interval uses plural(1) ("second"), which is why the two new tests expect different words for the same value. It's not this PR's job to change that, but worth knowing if someone asks why the tests differ.
Reviewed with help from Claude Code.
1AbdullahButt
left a comment
There was a problem hiding this comment.
The sign fix itself is right, and both Duration.in_words() and Interval.in_words() are covered. A rounding side effect is worth handling now that the sign isn't stripped, though. The value goes through :.2f, so anything between -0.005 s and 0 renders as negative zero:
>>> f"{-1000 / 1e6:.2f}", f"{-4999 / 1e6:.2f}"
('-0.00', '-0.00')So pendulum.duration(microseconds=-1000).in_words() would become "-0.00 seconds". Before this PR it was "0.00 seconds", which is also odd, but at least not signed. Options: round first and drop the sign when the rounded value is zero, e.g.
value = round(self.microseconds / 1e6, 2)
count = f"{value + 0.0:.2f}" # +0.0 turns -0.0 into 0.0or fall through to the microsecond unit when abs(microseconds) < 5000. A test with microseconds=-1000 would pin whichever you choose.
Separately, and pre-existing: Duration uses plural(0) ("seconds") while Interval uses plural(1) ("second"), which is why the two new tests expect different words for the same value. It's not this PR's job to change that, but worth knowing if someone asks why the tests differ.
Reviewed with help from Claude Code.
Values between -0.005 s and 0 were formatting as "-0.00" after the sign fix. Round to two decimals first and add 0.0 so -0.0 becomes 0.0.
|
Good catch on the rounded negative zero. I went with the first option: value = round(self.microseconds / 1e6, 2)
count = f"{value + 0.0:.2f}"in both Agreed the plural(0) vs plural(1) mismatch is pre-existing and out of scope here. |
Pull Request Check List
A negative duration shorter than a second loses its sign in
in_words():The same happens for an interval whose end is less than a second before its start (
'0.12 second'). Longer negative values already keep their sign (test_singular_negative_valuesexpects-1 day). The sub-second branch formattedabs(self.microseconds), andmicrosecondsis the only place the sign is kept for these values.The fix formats
self.microsecondswithoutabs()in bothDuration.in_words()andInterval.in_words(). Positive values and the zero case (0 microseconds) are unchanged.Tests: added
test_negative_subsecondstotests/duration/test_in_words.pyandtests/interval/test_in_words.py. Both fail before the fix and pass after it. The full suite has 1872 passed and 1 failed here, run withPENDULUM_EXTENSIONS=0. The failure istests/test_main.py::test_instance_with_aware_datetime_any_tzinfo, which also fails onmasterin this environment.ruff checkandruff format --check(v0.14.11, the pinned version) pass.I wrote this change with help from an AI assistant. I reviewed it and ran the tests listed above.