Dynamic Humanize and describe_multi Bug Fix #997

anishnya · 2021-06-27T19:07:27Z

Pull Request Checklist

Thank you for taking the time to improve Arrow! Before submitting your pull request, please check all appropriate boxes:

🧪 Added tests for changed code.
🛠️ All tests pass when run locally (run tox or make test to find out!).
🧹 All linting checks pass when run locally (run tox -e lint or make lint to find out!).
📚 Updated documentation for changed code.
⏩ Code is up-to-date with the master branch.

If you have any questions about your code changes or any of the points above, please submit your questions along with the pull request and we will try our best to help!

Description of Changes

Closes #996 and #973. I'll add the documentation for dynamic humanize after we make sure we like the approach we have here. Also, I'm linking #983 since I had to make some change to describe_multi that will be useful info for the locale implementation guide.

codecov · 2021-06-27T19:08:26Z

Codecov Report

Merging #997 (995ee01) into master (baebfff) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 995ee01 differs from pull request most recent head e3a7f93. Consider uploading reports for the commit e3a7f93 to get more accurate results

@@            Coverage Diff            @@
##            master      #997   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines         2223      2164   -59     
  Branches       350       345    -5     
=========================================
- Hits          2223      2164   -59

Impacted Files	Coverage Δ
arrow/arrow.py	`100.00% <100.00%> (ø)`
arrow/locales.py	`100.00% <100.00%> (ø)`
arrow/factory.py	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update baebfff...e3a7f93. Read the comment docs.

MarkKoz · 2021-08-07T02:49:53Z

arrow/arrow.py

@@ -1122,6 +1122,7 @@ def humanize(
        locale: str = DEFAULT_LOCALE,
        only_distance: bool = False,
        granularity: Union[_GRANULARITY, List[_GRANULARITY]] = "auto",
+        dynamic: bool = False,


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.

@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?

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.

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.

That sounds reasonable. Any thoughts on my name suggestion?

Errr throwing out a few ideas for the name, only_natural, minimal, drop_zeros. omit_zeros is fine as well.

I think omit_zeros is probably the best name

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.

It's more convenient to pass a simple short string than to pass a list of 7 strings. Resolve arrow-py#997

MarkKoz · 2021-08-08T04:19:12Z

arrow/arrow.py

+                if not timeframes and dynamic:
+                    raise ValueError(
+                        "All provided granulairty values produced an output of zero. "
+                        "Consider using smaller granularities, or set the dynamic flag to False. "
+                    )


How about defaulting to "just now" rather than raising an exception? If you imagine this being used with user input, it would pretty much be a requirement to always wrap it in a try-except due to this being raised.

Also, you misspelled granularity.

A user could have a delta of let's say 2 days but only have the granularity of ["year, "month", "week"]. If they had dynamic on, it would output "just now." I think it would be a better idea to error out, then to give an inaccurate answer in that scenario.

Ah, it doesn't convert in that case e.g. 2 days = 2/7ths of a week? That's a good point then.

Will this still raise the exception if all granularities are provided, but all values are 0, or will it actually display "just now" in that case? I think it should be able to do that.

In fact, it could show "just now" if all units evaluate to zero, regardless of which granularities are provided, not just if all are provided.

Though that would be inconsistent with the behaviour of e.g. granularity="year" returning '0 years' rather than 'just now'. On the other hand, it seems like it raising an error in those cases should be avoidable somehow.

Our main goal is that if you provide a granularity, you expect the output to contain said granularity (with the exception of the omit zeros/dynamic functionality). Trying to figure out whether we should or shouldn't adapt the output to include some other unit seems unnecessary when we already have the auto function in humanize.

Raising an error absolves arrow of having to make the tough decision of how to handle this. Letting the user handle the error gives them flexibility, but not all users might prefer that at the cost of having to practically always handle this error if they're dealing with unknown inputs.

It's a matter of which use case is more common: wanting custom behaviour to handle this edge case, or wanting to not have to think about it. Either way, the user can anticipate what the result will be by subtracting the times and manually inspecting the delta before calling humanize. If they see the delta will result in all zeros, they can handle it instead of relying on the default behaviour proposed below. Of course, that's not as convenient as just catching an exception, but I don't see a way to make both sides happy.

The most consistent solution may be to return zero in the smallest unit of the given granularity. This would ensure that while dynamic=True may omit some units in the given granularity, it will never introduce new units. Consider

>>> a = arrow.get(2021, 8, 8) >>> b = arrow.get(2021, 8, 10) >>> a.humanize(b, granularity=["year", "month", "week"]) '0 years 0 months and 0 weeks ago'

It has no problem omitting the "2 days" even though it's the only non-zero unit. This is arguably not very useful, but it's what the current behaviour is. There are probably use cases that need to strictly follow the granularity, and those users appreciate this behaviour. Anyway, following from this behaviour, it should then also be acceptable for this to happen

>>> a = arrow.get(2021, 8, 8) >>> b = arrow.get(2021, 8, 10) >>> a.humanize(b, granularity=["year", "month", "week"], dynamic=True) '0 weeks ago'

If the user has dynamic on, that is an expression of an intent to cut down on the zeros in the output. I'd say it's more practical to make a compromise to return 1 zero than to take a strict stance of "must have no zeros" and be forced to raise an exception.

We can only do so much, arrow in the end is a library meant to work together with the dev, not think or do things the dev might not be aware of and does behind the scenes without awareness and not raise an exception that the dev might even be expecting to see raised.

MarkKoz · 2021-08-08T04:21:13Z

arrow/arrow.py

+                        if dynamic and trunc(abs(value)) == 0:
+                            pass
+                        elif trunc(abs(value)) != 1:


Would it make any significant difference to save the value of trunc(abs(value)) rather than calculating it twice? This could also be said for the other parts of the diff that use trunc.

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.

krisfremen

Good work so far, I want to have another pass at it as I ran upon #1019 while testing.

Cheers!

arrow/arrow.py

krisfremen · 2021-08-09T06:31:44Z

arrow/arrow.py

+                        if dynamic and trunc(abs(value)) == 0:
+                            pass
+                        elif trunc(abs(value)) != 1:


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.

krisfremen · 2021-08-09T06:32:47Z

arrow/arrow.py

+                if not timeframes and dynamic:
+                    raise ValueError(
+                        "All provided granulairty values produced an output of zero. "
+                        "Consider using smaller granularities, or set the dynamic flag to False. "
+                    )


We can only do so much, arrow in the end is a library meant to work together with the dev, not think or do things the dev might not be aware of and does behind the scenes without awareness and not raise an exception that the dev might even be expecting to see raised.

krisfremen · 2021-08-09T06:34:44Z

arrow/arrow.py

@@ -1122,6 +1122,7 @@ def humanize(
        locale: str = DEFAULT_LOCALE,
        only_distance: bool = False,
        granularity: Union[_GRANULARITY, List[_GRANULARITY]] = "auto",
+        dynamic: bool = False,


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.

Co-authored-by: Kris Fremen <me@krisfremen.com>

Added dynamic humanize support and fix a big related to humanize

f34285a

anishnya requested review from jadchaar and krisfremen June 27, 2021 19:07

anishnya changed the title ~~Added dynamic humanize support and fix a big related to humanize~~ Dynamic Humanize and describe_multi Bug Fix Jun 27, 2021

anishnya added this to In progress in Release 1.2.0 Jun 28, 2021

anishnya removed this from In progress in Release 1.2.0 Jun 28, 2021

anishnya added this to In progress in Release 1.2.0 Jun 29, 2021

MarkKoz mentioned this pull request Aug 7, 2021

Add an "all" granularity to humanize #1014

Open

MarkKoz reviewed Aug 7, 2021

View reviewed changes

anishnya mentioned this pull request Aug 7, 2021

Empty List of Granularities For Humanize Doesn't Raise Value Error #1015

Closed

Anish Nyayachavadi added 2 commits August 7, 2021 13:25

Added Test Cases for Improved Coverage

4732ae0

Fixed Codecov issue

9ce1d98

MarkKoz added a commit to MarkKoz/arrow that referenced this pull request Aug 8, 2021

Add an "all" granularity to humanize

2506875

It's more convenient to pass a simple short string than to pass a list of 7 strings. Resolve arrow-py#997

MarkKoz mentioned this pull request Aug 8, 2021

Add an "all" granularity to humanize #1018

Open

5 tasks

MarkKoz reviewed Aug 8, 2021

View reviewed changes

Merge branch 'master' into dynamic-humanize

a4cd177

krisfremen reviewed Aug 9, 2021

View reviewed changes

Fixed spelling error within error message

cf9ea87

Co-authored-by: Kris Fremen <me@krisfremen.com>

anishnya requested review from krisfremen and systemcatch August 14, 2021 08:20

anishnya added 2 commits August 22, 2021 09:06

Merge branch 'master' into dynamic-humanize

995ee01

Merge branch 'master' into dynamic-humanize

e3a7f93

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic Humanize and describe_multi Bug Fix #997

Dynamic Humanize and describe_multi Bug Fix #997

anishnya commented Jun 27, 2021

codecov bot commented Jun 27, 2021 •

edited

MarkKoz Aug 7, 2021

anishnya Aug 7, 2021

MarkKoz Aug 7, 2021

krisfremen Aug 7, 2021

MarkKoz Aug 7, 2021

systemcatch Aug 8, 2021

anishnya Aug 8, 2021

krisfremen Aug 9, 2021

MarkKoz Aug 8, 2021

MarkKoz Aug 8, 2021

anishnya Aug 8, 2021

MarkKoz Aug 8, 2021

MarkKoz Aug 8, 2021 •

edited

MarkKoz Aug 8, 2021

anishnya Aug 9, 2021

MarkKoz Aug 9, 2021

krisfremen Aug 9, 2021

MarkKoz Aug 8, 2021 •

edited

krisfremen Aug 9, 2021

krisfremen left a comment

krisfremen Aug 9, 2021

krisfremen Aug 9, 2021

krisfremen Aug 9, 2021

Dynamic Humanize and describe_multi Bug Fix #997

Are you sure you want to change the base?

Dynamic Humanize and describe_multi Bug Fix #997

Conversation

anishnya commented Jun 27, 2021

Pull Request Checklist

Description of Changes

codecov bot commented Jun 27, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkKoz Aug 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkKoz Aug 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krisfremen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 27, 2021 •

edited

MarkKoz Aug 8, 2021 •

edited

MarkKoz Aug 8, 2021 •

edited