humanize icon indicating copy to clipboard operation
humanize copied to clipboard

* time.py, added iso 8601 support

Open velle opened this issue 2 years ago • 6 comments

Fixes https://github.com/python-humanize/humanize/issues/129.

  • time.py, added new function _parseiso(value)* relevant functions now call value = _parseiso(value)
  • wrote inline documentation, and short subparagraph to README.md
  • have not tested code in any way except the existing pytests which all succeeded
  • tox gives mypy errors. I was able to remove some, but not all.
  • tox gives lint errors. Have not figured out how to see what causes the errors.
  • tox gives griffe error, ast.BoolOp, but did so even before I edited code
  • not sure if Im using tox correctly. I simply run 'tox'

velle avatar Jul 09 '23 11:07 velle

Codecov Report

Merging #130 (d5eb5ac) into main (a8f8119) will increase coverage by 0.01%. The diff coverage is 100.00%.

:exclamation: Current head d5eb5ac differs from pull request most recent head 32014ab. Consider uploading reports for the commit 32014ab to get more accurate results

@@            Coverage Diff             @@
##             main     #130      +/-   ##
==========================================
+ Coverage   99.18%   99.20%   +0.01%     
==========================================
  Files           9        9              
  Lines         737      754      +17     
==========================================
+ Hits          731      748      +17     
  Misses          6        6              
Flag Coverage Δ
macos-latest 97.61% <100.00%> (+0.19%) :arrow_up:
ubuntu-latest 97.61% <100.00%> (+0.05%) :arrow_up:
windows-latest 96.15% <100.00%> (+0.08%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/humanize/time.py 100.00% <100.00%> (ø)
tests/test_time.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

codecov[bot] avatar Jul 09 '23 13:07 codecov[bot]

Hi Hugo. First of all, thanks for the feedback and for helping me learn this :) I will get back to the other points in your feedback later, but right now I need to figure out how to handle what is described below.

I started writing tests (some hours ago), and was almost done. But then I realized that humanize does not support datetime.time at all. I had written _parse_iso to also parse into datetime.time, when possible.

Now I have removed everything that has to do with datetime.time, and edited the documentation accordingly.

And I have committed and pushed this. This commit only removes the irrelevant time code. It does not include the tests I wrote. Since I imagined that you had to evaluate my commits, before ... merging? ... accepting? them into this iso8601 branch (or into this pull request?). The essence is: I decided to make this commit so it only concerns the removal of datetime.time.

Should I do more about this commit? Will this commit (and future commits) automatically be part of this pull request, until this pull request is closed or merged?

velle avatar Jul 09 '23 20:07 velle

This PR isn't accepted or merged yet (see the green "Open" at the top).

Yes, you can commit more things to your branch, and when you push them, they will automatically be part of this PR.

hugovk avatar Jul 09 '23 20:07 hugovk

Hi Hugo. As you noticed, I have been gone, first because of issues with my laptop, and then a week of holiday. But I'm back :)

I just made three commits: 1) Changed every call for _parseiso to _parse_iso. 2) Added tests for _parse_iso() in test_time.py. 3) And then added a few more tests to a few relevant test_x functions in test_time.py.

Sincerely.

velle avatar Jul 24 '23 07:07 velle

Thank you for the update. Please could you check the lint failures? They're mostly type hint things. Let me know if you'd like a hand.

flake8...................................................................Failed
- hook id: flake8
- exit code: 1

tests/test_time.py:102:32: F821 undefined name 'Object'
tests/test_time.py:102:50: F821 undefined name 'Object'
...
mypy.....................................................................Failed
- hook id: mypy
- exit code: 1

src/humanize/time.py:281: error: Incompatible types in assignment (expression has type "Union[date, float, str]", variable has type "Union[datetime, timedelta, float, str]")  [assignment]
src/humanize/time.py:281: error: Argument 1 to "_parse_iso" has incompatible type "Union[datetime, timedelta, float, str]"; expected "Union[date, datetime, float, str]"  [arg-type]
src/humanize/time.py:310: error: Incompatible types in assignment (expression has type "Union[date, float, str]", variable has type "Union[date, datetime]")  [assignment]
src/humanize/time.py:336: error: Incompatible types in assignment (expression has type "Union[date, float, str]", variable has type "Union[date, datetime]")  [assignment]
tests/test_time.py:102: error: Name "Object" is not defined  [name-defined]
Found 5 errors in 2 files (checked 10 source files)

hugovk avatar Jul 25 '23 08:07 hugovk

I managed to make all mypy errors disappear. But it was not easy to do this in an elegant way, when _parse_iso takes and returns a number of different types. The code I have pushed is the most elegant solution I could find, but it does contain the following check/cleanup in naturaltime:

_value = _parse_iso(value)
if type(_value) != dt.date:
    # naturaltime is not built to handle dt.date
    value = typing.cast(typing.Union[dt.datetime, dt.timedelta, float, str], _value)

One solution is modifying naturaltime so it would also accept dt.date as value. I looked into that but I wasnt sure what the intended behavior should be and if it made sense for all situations and values.

Another solution is going back to my original approach where the call for fromisoformat() is placed inside each of the naturalX-functions.

def naturaltime(value: dt.datetime | dt.timedelta | float | str)
    if isinstance(value,str):
        try:    value = dt.datetime.fromisoformat(value)
        except: pass

def naturalday(value: dt.date | dt.datetime):
    if isinstance(value,str):
        try:    value = dt.datetime.fromisoformat(value)
        except: pass
        try:    value = dt.date.fromisoformat(value)
        except: pass

def naturaldate(value: dt.date | dt.datetime):
    if isinstance(value,str):
        try:    value = dt.datetime.fromisoformat(value)
        except: pass
        try:    value = dt.date.fromisoformat(value)
        except: pass

Or yet another solution is to create two separate helper functions.

def _try_parse_datetime(value):
    try:
        return dt.datetime.fromisoformat(value)
    finally:
        return value
    
def _try_parse_date(value):
    try:
        return dt.datetime.fromisoformat(value)
    finally:
        return value

def naturaltime(value: dt.datetime | dt.timedelta | float | str)
    value = _try_parse_datetime(value)
    (...)

def naturalday(value: dt.date | dt.datetime):
    value = _try_parse_datetime(value)
    value = _try_parse_date(value)
    (...)

def naturaldate(value: dt.date | dt.datetime):
    value = _try_parse_datetime(value)
    value = _try_parse_date(value)
    (...)

I think I like both of these solutions better than what I pushed. But I'm not sure about anything.

velle avatar Jul 25 '23 14:07 velle