search

JohannesBuchner / imagehash

A Python Perceptual Image Hashing Module
BSD 2-Clause "Simplified" License
3.14k stars 328 forks source link

Added Python2 compatible type hints #167

Closed Avasam closed 2 years ago

Avasam commented 2 years ago

Closes #151

Just like #161, but addresses @JohannesBuchner concerns https://github.com/JohannesBuchner/imagehash/issues/151#issuecomment-977853870

I am wondering how to do this with the least amount of future maintenance effort. I am worried that the stub file may become outdated if not care is taken.

The refactor from a single file module to a package was necessary for the inclusion of py.typed, which lets type checkers know to not look for type stubs as types are inlined. See: https://peps.python.org/pep-0561/#packaging-type-information

Avasam commented 2 years ago

One issue with this is that Pyright won't recognize that I can substract two ImageHash, might be a bug on their end, I'll raise an issue.

Edit: This was an issue on my side and unrelated to these changes.

image

JohannesBuchner commented 2 years ago

Thank you for looking into this, @Avasam

JohannesBuchner commented 2 years ago

Do we have to add something in the testing / CI to check whether the typing stuff works? For example, a test that the types are not faulty, and a test that a calling code with the wrong type can be identified by a typing checker.

JohannesBuchner commented 2 years ago

the python3.9 CI run is complaining that numpy is too new: error: numpy 1.23.1 is installed but numpy<1.23.0,>=1.16.5 is required by {'scipy'}. Odd that conda installs this combination.

Avasam commented 2 years ago

Do we have to add something in the testing / CI to check whether the typing stuff works? For example, a test that the types are not faulty, and a test that a calling code with the wrong type can be identified by a typing checker.

You could run type checkers to ensure that the returned and used types match the annotations. Popular ones are mypy and pyright. Typeshed also tests against pytyped.

I am quite familiar with pyright so I could add it and configure it. A bit less so with the other two, but I don't think it's anything too complicated. Would you like me to add them as part of this PR or separately?

the python3.9 CI run is complaining that numpy is too new: error: numpy 1.23.1 is installed but numpy<1.23.0,>=1.16.5 is required by {'scipy'}. Odd that conda installs this combination.

I've noticed that with my other PR as well. Conda doesn't support Environment markers (which would be an easy fix), but it has something called Preprocessing selectors instead that we can try to use.

Avasam commented 2 years ago

the python3.9 CI run is complaining that numpy is too new: error: numpy 1.23.1 is installed but numpy<1.23.0,>=1.16.5 is required by {'scipy'}. Odd that conda installs this combination.

Unrelated to conda. The step that tested the setup.py install (which conda doesn't care about the existence of setup.py) was using a deprecated and obsolete way of installing.

$ python setup.py install running install /usr/share/miniconda/envs/test/lib/python3.9/site-packages/setuptools/command/install.py:34: SetuptoolsDeprecationWarning: setup.py install is deprecated. Use build and pip and other standards-based tools. /usr/share/miniconda/envs/test/lib/python3.9/site-packages/setuptools/command/easy_install.py:144: EasyInstallDeprecationWarning: easy_install command is deprecated. Use build and pip and other standards-based tools.

coveralls commented 2 years ago

Coverage Status

Coverage decreased (-4.5%) to 85.0% when pulling af1b868267d024698d48739680cce7a1f2077952 on Avasam:python2-typing into 0abd4878bdb3c2b7bd0a5ec58d1ffca530e70cec on JohannesBuchner:master.

JohannesBuchner commented 2 years ago

I am okay with merging this if it is ready from your side.

JohannesBuchner commented 2 years ago

Thank you for your work on this! I suppose I should make a release now.

Avasam commented 2 years ago

Thank you for your work on this! I suppose I should make a release now.

You're welcome! I learned a lot about typing since I opened the issue last year. So I decided to give it a go. And it's better to have it directly as part of the library rather than in typeshed.
I'm installing from github for now. git+https://github.com/JohannesBuchner/imagehash#egg=ImageHash

JohannesBuchner commented 2 years ago

release 4.3 is out, please test it and let me know whether it works for you.

I made some substantial changes in the code regarding python version checking, hope it did not break things. If it did, we need better CI checks.