Repository: arrow-py/arrow

Issue:
humanize() rounding problem
<!--
Thanks for taking the time to submit this bug report.

Please provide us with a detailed description of the
bug and a bit of information about your system.
-->

## Issue Description
Not sure if this is wanted or a bug. `humanize()` returns "a week ago" for deltas < 7 days.
For example:
```
>>> arrow.utcnow().shift(days=-6, hours=-9).humanize()
'6 days ago'
>>> arrow.utcnow().shift(days=-6, hours=-10).humanize()
'a week ago'
```
I would expect "a week ago" to be returned after 7 days have passed.

## System Info

- 🖥 **OS name and version**: Fedora 32
- 🐍 **Python version**: Python 3.8.5
- 🏹 **Arrow version**: arrow 0.16.0


Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/arrow-py/arrow/pull/868?src=pr&el=h1) Report
> Merging [#868](https://codecov.io/gh/arrow-py/arrow/pull/868?src=pr&el=desc) into [master](https://codecov.io/gh/arrow-py/arrow/commit/7cccf6a7034bea1616d1df413f90bf902be77a57?el=desc) will **not change** coverage.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/arrow-py/arrow/pull/868/graphs/tree.svg?width=650&height=150&src=pr&token=4EkGnFsIGn)](https://codecov.io/gh/arrow-py/arrow/pull/868?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##            master      #868   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9         9           
  Lines         1809      1816    +7     
  Branches       312       312           
=========================================
+ Hits          1809      1816    +7     
```


| [Impacted Files](https://codecov.io/gh/arrow-py/arrow/pull/868?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [arrow/arrow.py](https://codecov.io/gh/arrow-py/arrow/pull/868/diff?src=pr&el=tree#diff-YXJyb3cvYXJyb3cucHk=) | `100.00% <100.00%> (ø)` | |
| [arrow/util.py](https://codecov.io/gh/arrow-py/arrow/pull/868/diff?src=pr&el=tree#diff-YXJyb3cvdXRpbC5weQ==) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/arrow-py/arrow/pull/868?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/arrow-py/arrow/pull/868?src=pr&el=footer). Last update [7cccf6a...9dba7d7](https://codecov.io/gh/arrow-py/arrow/pull/868?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

systemcatch: > Looks good but I think we should get rid of the magic constants and instead refer to the static class constants.

👍 

> Also are you planning to refactor and break up the humanize function in another PR?

Yeah that can wait.

systemcatch: Right so I've made changes based on both your suggestions, however there are still some problems to work out.

For example:
```shell
(arrow) chris@ThinkPad:~/arrow$ python
Python 3.8.3 (default, Jul  7 2020, 18:57:36) 
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import arrow
>>> dt=arrow.utcnow()
>>> later=dt.shift(years=1)
>>> later.humanize(dt)
'in a year'
>>> later.humanize(dt, granularity="year")
'in 0 years'
```
This happens because `_SECONDS_PER_YEAR` is `60*60*24*365.25` whereas I've used `60*60*24*365`. I tend to think it's not worth worrying about leap years in humanize. There will be a similar problem with months which I don't think is such an issue.
jadchaar: > 
> 
> Right so I've made changes based on both your suggestions, however there are still some problems to work out.
> 
> For example:
> 
> ```shell
> (arrow) chris@ThinkPad:~/arrow$ python
> Python 3.8.3 (default, Jul  7 2020, 18:57:36) 
> [GCC 9.3.0] on linux
> Type "help", "copyright", "credits" or "license" for more information.
> >>> import arrow
> >>> dt=arrow.utcnow()
> >>> later=dt.shift(years=1)
> >>> later.humanize(dt)
> 'in a year'
> >>> later.humanize(dt, granularity="year")
> 'in 0 years'
> ```
> 
> This happens because `_SECONDS_PER_YEAR` is `60*60*24*365.25` whereas I've used `60*60*24*365`. I tend to think it's not worth worrying about leap years in humanize. There will be a similar problem with months which I don't think is such an issue.

Do you think we should define a class in constants called Humanize constants? Or even a static inner class of the Arrow object? Because I don't think these .25 adjustments for leap years should be used for humanize. Thoughts?
systemcatch: > Do you think we should define a class in constants called Humanize constants? Or even a static inner class of the Arrow object? Because I don't think these .25 adjustments for leap years should be used for humanize. Thoughts?

Maybe we just change `_SECS_PER_YEAR` to `60*60*24*365`? It's not used anywhere else apart from humanize.
jadchaar: > > Do you think we should define a class in constants called Humanize constants? Or even a static inner class of the Arrow object? Because I don't think these .25 adjustments for leap years should be used for humanize. Thoughts?
> 
> Maybe we just change `_SECS_PER_YEAR` to `60*60*24*365`? It's not used anywhere else apart from humanize.

Yeah I think it makes most sense to accommodate it for where it is being used. What do you think?
systemcatch: @jadchaar I've changed the value to `60*60*24*365`
systemcatch: @jadchaar I made your suggested changes, we should be good to go here.

Patch:
diff --git a/arrow/arrow.py b/arrow/arrow.py
index 4fe9541..f3706b5 100644
--- a/arrow/arrow.py
+++ b/arrow/arrow.py
@@ -72,7 +72,7 @@ class Arrow(object):
     _SECS_PER_DAY = float(60 * 60 * 24)
     _SECS_PER_WEEK = float(60 * 60 * 24 * 7)
     _SECS_PER_MONTH = float(60 * 60 * 24 * 30.5)
-    _SECS_PER_YEAR = float(60 * 60 * 24 * 365.25)
+    _SECS_PER_YEAR = float(60 * 60 * 24 * 365)
 
     def __init__(
         self,
@@ -1012,42 +1012,41 @@ class Arrow(object):
                 if diff < 10:
                     return locale.describe("now", only_distance=only_distance)
 
-                if diff < 45:
+                if diff < self._SECS_PER_MINUTE:
                     seconds = sign * delta
                     return locale.describe(
                         "seconds", seconds, only_distance=only_distance
                     )
 
-                elif diff < 90:
+                elif diff < self._SECS_PER_MINUTE * 2:
                     return locale.describe("minute", sign, only_distance=only_distance)
-                elif diff < 2700:
-                    minutes = sign * int(max(delta / 60, 2))
+                elif diff < self._SECS_PER_HOUR:
+                    minutes = sign * int(max(delta / self._SECS_PER_MINUTE, 2))
                     return locale.describe(
                         "minutes", minutes, only_distance=only_distance
                     )
 
-                elif diff < 5400:
+                elif diff < self._SECS_PER_HOUR * 2:
                     return locale.describe("hour", sign, only_distance=only_distance)
-                elif diff < 79200:
-                    hours = sign * int(max(delta / 3600, 2))
+                elif diff < self._SECS_PER_DAY:
+                    hours = sign * int(max(delta / self._SECS_PER_HOUR, 2))
                     return locale.describe("hours", hours, only_distance=only_distance)
-
-                # anything less than 48 hours should be 1 day
-                elif diff < 172800:
+                elif diff < self._SECS_PER_DAY * 2:
                     return locale.describe("day", sign, only_distance=only_distance)
-                elif diff < 554400:
-                    days = sign * int(max(delta / 86400, 2))
+                elif diff < self._SECS_PER_WEEK:
+                    days = sign * int(max(delta / self._SECS_PER_DAY, 2))
                     return locale.describe("days", days, only_distance=only_distance)
 
-                elif diff < 907200:
+                elif diff < self._SECS_PER_WEEK * 2:
                     return locale.describe("week", sign, only_distance=only_distance)
-                elif diff < 2419200:
-                    weeks = sign * int(max(delta / 604800, 2))
+                elif diff < self._SECS_PER_MONTH:
+                    weeks = sign * int(max(delta / self._SECS_PER_WEEK, 2))
                     return locale.describe("weeks", weeks, only_distance=only_distance)
 
-                elif diff < 3888000:
+                elif diff < self._SECS_PER_MONTH * 2:
                     return locale.describe("month", sign, only_distance=only_distance)
-                elif diff < 29808000:
+                elif diff < self._SECS_PER_YEAR:
+                    # TODO revisit for humanization during leap years
                     self_months = self._datetime.year * 12 + self._datetime.month
                     other_months = dt.year * 12 + dt.month
 
@@ -1057,10 +1056,10 @@ class Arrow(object):
                         "months", months, only_distance=only_distance
                     )
 
-                elif diff < 47260800:
+                elif diff < self._SECS_PER_YEAR * 2:
                     return locale.describe("year", sign, only_distance=only_distance)
                 else:
-                    years = sign * int(max(delta / 31536000, 2))
+                    years = sign * int(max(delta / self._SECS_PER_YEAR, 2))
                     return locale.describe("years", years, only_distance=only_distance)
 
             elif util.isstr(granularity):


Test patch:
diff --git a/tests/test_arrow.py b/tests/test_arrow.py
index b0bd20a..872edc0 100644
--- a/tests/test_arrow.py
+++ b/tests/test_arrow.py
@@ -1772,11 +1772,12 @@ class TestArrowHumanize:
             )
             == "37 months and 4 weeks"
         )
+        # this will change when leap years are implemented
         assert (
             self.now.humanize(
                 later108onlydistance, only_distance=True, granularity=["year", "second"]
             )
-            == "3 years and 5327200 seconds"
+            == "3 years and 5392000 seconds"
         )
 
         one_min_one_sec_ago = self.now.shift(minutes=-1, seconds=-1)
@@ -1909,16 +1910,26 @@ class TestArrowHumanize:
         assert self.now.humanize(later, only_distance=True) == "2 weeks"
         assert later.humanize(self.now, only_distance=True) == "2 weeks"
 
+    @pytest.mark.xfail(reason="known issue with humanize month limits")
     def test_month(self):
 
         later = self.now.shift(months=1)
 
+        # TODO this test now returns "4 weeks ago", we need to fix this to be correct on a per month basis
         assert self.now.humanize(later) == "a month ago"
         assert later.humanize(self.now) == "in a month"
 
         assert self.now.humanize(later, only_distance=True) == "a month"
         assert later.humanize(self.now, only_distance=True) == "a month"
 
+    def test_month_plus_4_days(self):
+
+        # TODO needed for coverage, remove when month limits are fixed
+        later = self.now.shift(months=1, days=4)
+
+        assert self.now.humanize(later) == "a month ago"
+        assert later.humanize(self.now) == "in a month"
+
     def test_months(self):
 
         later = self.now.shift(months=2)
@@ -1954,7 +1965,7 @@ class TestArrowHumanize:
 
         result = arw.humanize(self.datetime)
 
-        assert result == "in 2 years"
+        assert result == "in a year"
 
     def test_arrow(self):
 
@@ -1998,6 +2009,16 @@ class TestArrowHumanize:
 
         assert result == "just now"
 
+    def test_week_limit(self):
+        # regression test for issue #848
+        arw = arrow.Arrow.utcnow()
+
+        later = arw.shift(weeks=+1)
+
+        result = arw.humanize(later)
+
+        assert result == "a week ago"
+
     def test_untranslated_granularity(self, mocker):
 
         arw = arrow.Arrow.utcnow()
@@ -2033,7 +2054,7 @@ class TestArrowHumanizeTestsWithLocale:
 
         result = arw.humanize(self.datetime, locale="ru")
 
-        assert result == "2 года назад"
+        assert result == "год назад"
 
 
 class TestArrowIsBetween: