-
-
Notifications
You must be signed in to change notification settings - Fork 678
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dynamic Humanize and describe_multi Bug Fix #997
base: master
Are you sure you want to change the base?
Changes from 1 commit
f34285a
4732ae0
9ce1d98
a4cd177
cf9ea87
995ee01
e3a7f93
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1122,6 +1122,7 @@ def humanize( | |
locale: str = DEFAULT_LOCALE, | ||
only_distance: bool = False, | ||
granularity: Union[_GRANULARITY, List[_GRANULARITY]] = "auto", | ||
dynamic: bool = False, | ||
) -> str: | ||
"""Returns a localized, humanized representation of a relative difference in time. | ||
|
||
|
@@ -1264,7 +1265,11 @@ def gather_timeframes(_delta: float, _frame: TimeFrameLiteral) -> float: | |
if _frame in granularity: | ||
value = sign * _delta / self._SECS_MAP[_frame] | ||
_delta %= self._SECS_MAP[_frame] | ||
if trunc(abs(value)) != 1: | ||
|
||
# If user chooses dynamic and the display value is 0 don't subtract | ||
if dynamic and trunc(abs(value)) == 0: | ||
pass | ||
elif trunc(abs(value)) != 1: | ||
Comment on lines
+1282
to
+1284
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make any significant difference to save the value of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Decent catch. Profiling the code, you'd need to run about 10k of the trunc(abs()) calls to even come close to seeing a 1ms difference. |
||
timeframes.append( | ||
(cast(TimeFrameLiteral, _frame + "s"), value) | ||
) | ||
|
@@ -1285,12 +1290,18 @@ def gather_timeframes(_delta: float, _frame: TimeFrameLiteral) -> float: | |
for frame in frames: | ||
delta = gather_timeframes(delta, frame) | ||
|
||
if len(timeframes) < len(granularity): | ||
if len(timeframes) < len(granularity) and not dynamic: | ||
raise ValueError( | ||
"Invalid level of granularity. " | ||
"Please select between 'second', 'minute', 'hour', 'day', 'week', 'month' or 'year'." | ||
) | ||
|
||
# Needed for the case of dynamic usage (could end up with only one frame unit) | ||
if len(timeframes) == 1: | ||
return locale.describe( | ||
timeframes[0][0], delta, only_distance=only_distance | ||
) | ||
|
||
return locale.describe_multi(timeframes, only_distance=only_distance) | ||
|
||
except KeyError as e: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this False by default to avoid a breaking change? That's understandable, but also disappointing since to me, the dynamic behaviour seems much more useful than outputting a bunch of zeros for units.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarkKoz, yes we left as False by default in order to avoid changing the behaviour of humanize drastically. I agree that it would make more sense to leave it as True by default however. @jadchaar @krisfremen @systemcatch what are your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the least, I hope changing this can be considered for the next major release (assuming you're following SemVer).
Unrelated:
dynamic
isn't a good name — it's vague and non-self-descriptive.omit_zeros
or something similar would be clearer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should leave it as False for the time being, do a warning for changing behavior and change it to default True after a few versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds reasonable. Any thoughts on my name suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errr throwing out a few ideas for the name,
only_natural
,minimal
,drop_zeros
.omit_zeros
is fine as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think omit_zeros is probably the best name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I think the
dynamic
naming is fine, as IMO it indicates it will use dynamically any of the granularity fields that are specified, as the time progresses or is shifted.Although, omit_zeroes is a nice alternative name, I would prefer dynamic.