arrow-py / arrow

🏹 Better dates & times for Python
https://arrow.readthedocs.io
Apache License 2.0
8.71k stars 673 forks source link

Change humanize() granularity="auto" limits #868

Closed systemcatch closed 3 years ago

systemcatch commented 3 years ago

Pull Request Checklist

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

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

First attempt at updating the humanize() auto limits to avoid weird results.

Need to check failing tests and finish the month/months limit.

closes #848

codecov[bot] commented 3 years ago

Codecov Report

Merging #868 (7b7067a) into master (fe9602e) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #868   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9         9           
  Lines         1810      1810           
  Branches       312       312           
=========================================
  Hits          1810      1810           
Impacted Files Coverage Δ
arrow/arrow.py 100.00% <100.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 fe9602e...7b7067a. Read the comment docs.

systemcatch commented 3 years ago

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 commented 3 years ago

Right so I've made changes based on both your suggestions, however there are still some problems to work out.

For example:

(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 commented 3 years ago

Right so I've made changes based on both your suggestions, however there are still some problems to work out.

For example:

(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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@jadchaar I've changed the value to 60*60*24*365

edit: spotted one small typo, will fix then we should be good to go.

systemcatch commented 3 years ago

@jadchaar I made your suggested changes, we should be good to go here.