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

Added Python2 compatible type hints #167

Merged
merged 4 commits into from
Aug 31, 2022
Merged

Added Python2 compatible type hints #167

merged 4 commits into from
Aug 31, 2022

Conversation

Avasam
Copy link

@Avasam Avasam commented Aug 29, 2022

Closes #151

Just like #161, but addresses @JohannesBuchner concerns #151 (comment)

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 Avasam mentioned this pull request Aug 29, 2022
@Avasam
Copy link
Author

Avasam commented Aug 29, 2022

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
Copy link
Owner

Thank you for looking into this, @Avasam

@JohannesBuchner
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

Avasam commented Aug 30, 2022

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
Copy link
Author

Avasam commented Aug 30, 2022

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
Copy link

coveralls commented Aug 30, 2022

Coverage Status

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

@JohannesBuchner
Copy link
Owner

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

@JohannesBuchner JohannesBuchner merged commit 9b7bcf7 into JohannesBuchner:master Aug 31, 2022
@JohannesBuchner
Copy link
Owner

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

@Avasam
Copy link
Author

Avasam commented Sep 3, 2022

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

@Avasam Avasam deleted the python2-typing branch September 3, 2022 20:05
@JohannesBuchner
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please add type hints or type stubs
3 participants