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

Add type hints #15

Merged
merged 12 commits into from Jun 13, 2022
Merged

Add type hints #15

merged 12 commits into from Jun 13, 2022

Conversation

hugovk
Copy link
Member

@hugovk hugovk commented May 3, 2022

Changes proposed in this pull request:

TODO

  • Check the docs failure in mkdocstrings:
ERROR    -  mkdocstrings: name 'TypeAlias' is not defined
Traceback (most recent call last):

https://github.com/python-humanize/humanize/runs/6276428220?check_suite_focus=true

@pawamoy
Copy link

pawamoy commented May 3, 2022

Hmmm, I was going to say "get_type_hints + pytkdocs won't work on python less than 3.9" but your pipeline ran on python 3.10 🤔

@hugovk
Copy link
Member Author

hugovk commented May 3, 2022

typing.TypeAlias is new in 3.10, maybe mkdocstrings / pytkdocs doesn't support it yet?

https://docs.python.org/3/library/typing.html#typing.TypeAlias

@pawamoy
Copy link

pawamoy commented May 3, 2022

Maybe it's simply because your TypeAlias import is type guarded? Therefore not available at runtime for get_type_hints to pick it up? pytkdocs doesn't do anything special with types (it has no particular knowledge of any other types).

Could you try to patch pytkdocs' loader module to add the future annotations import and see if it keeps failing?

@hugovk
Copy link
Member Author

hugovk commented May 4, 2022

Maybe it's simply because your TypeAlias import is type guarded? Therefore not available at runtime for get_type_hints to pick it up? pytkdocs doesn't do anything special with types (it has no particular knowledge of any other types).

Removing if TYPE_CHECKING: fixes it. I'd rather not remove it, because it would mean we need to add typing_extensions as a runtime library dependency, when it's only needed when checking types.

Could you try to patch pytkdocs' loader module to add the future annotations import and see if it keeps failing?

This doesn't fix it.

Copy link
Contributor

@coiax coiax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, couple of observations, but none of them blocking. 👍

tests/test_time.py Outdated Show resolved Hide resolved
tests/test_number.py Show resolved Hide resolved
@@ -32,7 +33,7 @@
([10**26 * 30, True, False, "%.3f"], "2481.542 YiB"),
],
)
def test_naturalsize(test_args, expected):
def test_naturalsize(test_args: list[int] | list[int | bool], expected: str) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very specific type hint for something that's just going to be slapped into argument unpacking. 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's a bit of a jumble! Will replace with list[typing.Any]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, with this:

diff --git a/tests/test_filesize.py b/tests/test_filesize.py
index 0119d58..857a958 100644
--- a/tests/test_filesize.py
+++ b/tests/test_filesize.py
@@ -3,6 +3,8 @@
 """Tests for filesize humanizing."""
 from __future__ import annotations
 
+import typing
+
 import pytest
 
 import humanize
@@ -33,7 +35,7 @@ import humanize
         ([10**26 * 30, True, False, "%.3f"], "2481.542 YiB"),
     ],
 )
-def test_naturalsize(test_args: list[int] | list[int | bool], expected: str) -> None:
+def test_naturalsize(test_args: list[typing.Any], expected: str) -> None:
     assert humanize.naturalsize(*test_args) == expected
 
     args_with_negative = test_args

The mypy fails when run from pre-commit but only as part of the Git commit:

$ gc -m "Use typing.Any"
pyupgrade................................................................Passed
black....................................................................Passed
isort....................................................................Passed
autoflake................................................................Passed
flake8...................................................................Passed
check blanket noqa.......................................................Passed
check for merge conflicts................................................Passed
check toml...........................................(no files to check)Skipped
check yaml...........................................(no files to check)Skipped
fix end of files.........................................................Passed
pydocstyle...........................................(no files to check)Skipped
mypy.....................................................................Failed
- hook id: mypy
- exit code: 1

tests/test_filesize.py:10: error: Cannot find implementation or library stub for module named "humanize"
tests/test_filesize.py:10: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
Found 1 error in 1 file (checked 1 source file)

setup-cfg-fmt........................................(no files to check)Skipped
pyproject-fmt........................................(no files to check)Skipped
tox-ini-fmt..........................................(no files to check)Skipped

Fine when run in pre-commit or directly as mypy:

$ pre-commit run --all-files
pyupgrade................................................................Passed
black....................................................................Passed
isort....................................................................Passed
autoflake................................................................Passed
flake8...................................................................Passed
check blanket noqa.......................................................Passed
check for merge conflicts................................................Passed
check toml...............................................................Passed
check yaml...............................................................Passed
fix end of files.........................................................Passed
pydocstyle...............................................................Passed
mypy.....................................................................Passed
setup-cfg-fmt............................................................Passed
pyproject-fmt............................................................Passed
tox-ini-fmt..............................................................Passed
$ mypy --strict .
Success: no issues found in 11 source files

Maybe I'll just leave the jumble...

src/humanize/time.py Show resolved Hide resolved
src/humanize/time.py Outdated Show resolved Hide resolved
Co-authored-by: coiax <yellowbounder@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented May 4, 2022

Codecov Report

Merging #15 (f550e98) into main (0c177f7) will decrease coverage by 0.59%.
The diff coverage is 97.36%.

@@            Coverage Diff             @@
##             main      #15      +/-   ##
==========================================
- Coverage   99.68%   99.08%   -0.60%     
==========================================
  Files           9        9              
  Lines         635      658      +23     
==========================================
+ Hits          633      652      +19     
- Misses          2        6       +4     
Flag Coverage Δ
macos-latest 97.87% <97.36%> (-0.56%) ⬇️
ubuntu-latest 97.87% <97.36%> (-0.56%) ⬇️
windows-latest 97.56% <97.36%> (-0.55%) ⬇️

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

Impacted Files Coverage Δ
src/humanize/number.py 96.77% <86.95%> (-2.37%) ⬇️
src/humanize/time.py 99.53% <98.18%> (-0.47%) ⬇️
src/humanize/__init__.py 100.00% <100.00%> (ø)
src/humanize/filesize.py 100.00% <100.00%> (ø)
src/humanize/i18n.py 98.11% <100.00%> (+0.03%) ⬆️
tests/test_filesize.py 100.00% <100.00%> (ø)
tests/test_i18n.py 100.00% <100.00%> (ø)
tests/test_number.py 100.00% <100.00%> (ø)
tests/test_time.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 0c177f7...f550e98. Read the comment docs.

@pawamoy
Copy link

pawamoy commented May 6, 2022

Removing if TYPE_CHECKING: fixes it. I'd rather not remove it, because it would mean we need to add typing_extensions as a runtime library dependency, when it's only needed when checking types.

Of course, completely understandable.

This doesn't fix it.

Erm, too bad. I'm not sure what is required at this point to make get_type_hints work. I'll need to investigate a bit more. Thank you for trying!

@hugovk
Copy link
Member Author

hugovk commented Jun 11, 2022

@pawamoy Hi! Any news on this? Do you have an upstream issue to follow?

@pawamoy
Copy link

pawamoy commented Jun 11, 2022

No news, no issue (feel free to open them in relevant repositories). Didn't take the time to investigate. At this point I'd recommend trying the new handler, which does not use introspection but rather visits the AST (I don't know if you already tried it or not).

@hugovk
Copy link
Member Author

hugovk commented Jun 11, 2022

I didn't try the new handler but happy to give it a go! What do we need to change to try it?

@pawamoy
Copy link

pawamoy commented Jun 12, 2022

You just need to depend on mkdocstrings-python instead of mkdocstrings-python-legacy, or to use the python extra instead of the python-legacy one 🙂

More information about the two handlers: https://mkdocstrings.github.io/handlers/overview/#about-the-python-handlers.
Highlight: the new handler does not support documenting inherited members yet, and there are known issues with wildcard imports.

@hugovk
Copy link
Member Author

hugovk commented Jun 12, 2022

Thanks, that works!

@hugovk hugovk merged commit 7688f20 into python-humanize:main Jun 13, 2022
@hugovk hugovk deleted the typing branch June 13, 2022 16:21
@sodul
Copy link

sodul commented Jul 7, 2022

@hugovk the times in seconds are typed as int but they should be float. These changes are failing mypy on our side.

@hugovk
Copy link
Member Author

hugovk commented Jul 8, 2022

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

@sodul
Copy link

sodul commented Jul 8, 2022

@hugovk We got the issue with humanize==4.2.3, rolling back to humanize==4.1.0 helped.

Sample error:

error: Argument 1 to "precisedelta" has incompatible type "float"; expected "Union[timedelta, int]"

I took an other look and in a way the int type properly reflects the actual behavior:

>>> import humanize
>>> humanize.precisedelta(0.05)
'0 seconds'

>>> humanize.precisedelta(1.05)
'1 second'

>>> humanize.precisedelta(0.05, minimum_unit="microseconds")
'0 microseconds'

>>> humanize.precisedelta(1.05, minimum_unit="microseconds")
'1 second'

>>> humanize.precisedelta(1.05, minimum_unit="milliseconds")
'1 second'

>>> humanize.precisedelta(0.05, minimum_unit="milliseconds")
'0 milliseconds'

When the seconds are passed as a float it works, but only the integer part is used. That's because _date_and_delta() does value = int(value) but I think that it could do value = float(value) instead, and then only if the type is not already a number (I suspect the intent was to cast strings to a number value).

So at the end of the day ... the type annotation is reflecting the behavior. Personally I believe float is fine (it accepts int types as a subset of float), and that the code could even benefit to actually support fractions of seconds when minimum_unit allows for it.

For now I can update my code to cast to int in order to satisfy mypy, even though it does not feel right:
humanize.precisedelta(int(seconds))

@nuztalgia
Copy link
Contributor

@sodul I have an open PR (#39) that I believe will fix the mypy issue. It also adds more precise support for float values:

>>> precisedelta(0.01, "seconds", format="%0.3f")
BEFORE:  "0 seconds"
AFTER:   "0.010 seconds"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: Added For new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants