Skip to content
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

Inconsistencies between documentation/annotations and behavior #28

Closed
serhiy-storchaka opened this issue Jun 24, 2022 · 9 comments
Closed
Labels
bug Something isn't working

Comments

@serhiy-storchaka
Copy link

serhiy-storchaka commented Jun 24, 2022

naturaltime() supports timedelta.

It is not documented, but humanize.naturaltime() accepts timedelta. I used it for years until type annotations were added, and MyPy started to complain. timedelta is still accepted at runtime.

>>> import datetime, humanize
>>> humanize.naturaltime(datetime.timedelta(days=5, hours=3))
'5 days ago'
>>> humanize.naturaltime(datetime.timedelta(hours=123))
'5 days ago'

I expect one of two things:

  1. Document that humanize.naturaltime() accepts timedelta, not only datetime | int and update type annotation.
  2. Make humanize.naturaltime() rejecting timedelta (of course, after some deprecation period).

Option 1 is more compatible, but option 2 looks more logical.

naturaltime() and naturaldelta() accept not only int, but float too.

>>> humanize.naturaltime(23.5)
'23 seconds ago'
>>> humanize.naturaldelta(23.5)
'23 seconds'

It is expected, since timestamp in Python is float, but it should be documented, and annotations should be updated.

naturaltime(), naturaldelta() , naturaldate() and naturalday() accept arbitrary object without error.

>>> humanize.naturaltime([1, 2, 3])
'[1, 2, 3]'
>>> humanize.naturaldelta([1, 2, 3])
'[1, 2, 3]'
>>> humanize.naturaldate([1, 2, 3])
'[1, 2, 3]'
>>> humanize.naturalday([1, 2, 3])
'[1, 2, 3]'

It is unsafe, because allows programming errors to slip unnoticed. humanize.naturalsize() raises an error, as expected.

Humanize 4.2.1

@hugovk
Copy link
Member

hugovk commented Jun 25, 2022

naturaltime() supports timedelta.

...

I expect one of two things:

  1. Document that humanize.naturaltime() accepts timedelta, not only datetime | int and update type annotation.
  2. Make humanize.naturaltime() rejecting timedelta (of course, after some deprecation period).

Option 1 is more compatible, but option 2 looks more logical.

Let's do option 1, possibly with some tests.

Option 2 with deprecation could be sensible too, let's also check what the Django naturaltime does.

Feedback from others welcome!


naturaltime() and naturaldelta() accept not only int, but float too.

...

It is expected, since timestamp in Python is float, but it should be documented, and annotations should be updated.

Yep, let's update those.


naturaltime(), naturaldelta() , naturaldate() and naturalday() accept arbitrary object without error.

...

It is unsafe, because allows programming errors to slip unnoticed. humanize.naturalsize() raises an error, as expected.

Yeah, I do find it a little odd to catch things like ValueError, TypeError, AttributeError and then just return str(value), but it's done that for a long time and I didn't want to break backwards compatibility.

Maybe we should also check what the Django ones do and consider deprecating and removing in a major bump.

Feedback from others also welcome here!

@hugovk
Copy link
Member

hugovk commented Jun 27, 2022

Let's do option 1, possibly with some tests.

Released in 4.2.2:

@nuztalgia
Copy link
Contributor

naturaltime() and naturaldelta() accept not only int, but float too.

If I'm not mistaken, this looks like the issue that was fixed in 4.2.2. (And thank you for that!)


naturaltime() supports timedelta.

  1. Document that humanize.naturaltime() accepts timedelta, not only datetime | int and update type annotation.
  2. Make humanize.naturaltime() rejecting timedelta (of course, after some deprecation period).

Let's do option 1, possibly with some tests.

This issue, however, seems to still be open as of 4.2.2. If option 1 is still on the table, that would (in my opinion) be ideal!

Here's an example of how I'm using naturaltime() with a timedelta, along with # noinspection PyTypeChecker as a workaround for now. 😅 As far as I can tell, naturaltime() seems to be the most intuitive function for my desired output (e.g. "1 year, 10 months ago").

Please let me know if I'm misunderstanding something, or if that sounds reasonable! I'm happy to try to put together a PR to add documentation/annotations for timedelta, if that's the direction you want to take.

@hugovk
Copy link
Member

hugovk commented Jun 28, 2022

naturaltime() and naturaldelta() accept not only int, but float too.

If I'm not mistaken, this looks like the issue that was fixed in 4.2.2. (And thank you for that!)

Ah, correct, it was indeed the float issue fixed in 4.2.2, sorry for the mixup :)


naturaltime() supports timedelta.

  1. Document that humanize.naturaltime() accepts timedelta, not only datetime | int and update type annotation.
  2. Make humanize.naturaltime() rejecting timedelta (of course, after some deprecation period).

Let's do option 1, possibly with some tests.

This issue, however, seems to still be open as of 4.2.2. If option 1 is still on the table, that would (in my opinion) be ideal!

Here's an example of how I'm using naturaltime() with a timedelta, along with # noinspection PyTypeChecker as a workaround for now. 😅 As far as I can tell, naturaltime() seems to be the most intuitive function for my desired output (e.g. "1 year, 10 months ago").

Please let me know if I'm misunderstanding something, or if that sounds reasonable! I'm happy to try to put together a PR to add documentation/annotations for timedelta, if that's the direction you want to take.

Yes, option 1 is still on the table, PR welcome! Please include a test case passing a timedelta to naturaltime(), if there's not already.

@hugovk
Copy link
Member

hugovk commented Jun 30, 2022

And the naturaldelta() + timedelta fix has been released in 4.2.3. Thanks!

@hugovk hugovk added the bug Something isn't working label Jun 30, 2022
@sodul
Copy link

sodul commented Jul 7, 2022

The type for seconds should be float and not int as introduced in #15. This actually breaks mypy for us.

@hugovk
Copy link
Member

hugovk commented Jul 8, 2022

@sodul In which function? And what version of Humanize are you using?

@sodul
Copy link

sodul commented Jul 11, 2022

@hugovk I posted details in the conversation for #15.

@hugovk
Copy link
Member

hugovk commented Apr 13, 2023

Most if not all of these should be addressed, please open a new issue if there's something further. Thanks!

@hugovk hugovk closed this as completed Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants