Add type hinting with mypy?

healpy / healpy

Python healpix maps tools

healpy.readthedocs.org

GNU General Public License v2.0

264 stars 187 forks source link

Add type hinting with mypy? #467

Open DanielLenz opened 6 years ago

DanielLenz commented 6 years ago

I've been adding type hinting for a lot of projects recently, and found that if combined with travis CI, can significantly help in maintaining and debugging.

This would mean that e.g. the following changes would be made:

def nside2resol(nside, arcmin=False):
    ...

def nside2resol(nside: int, arcmin: bool=False):
    ...

On top of that, the travis build would include the mypy linter.

I'd be happy to set this up and do most of the changes, if you think it would be a sensible addition.

mreineck commented 6 years ago

I think such a change could be extremely useful, but I have two questions:

with the syntax you are proposing in the example above, wouldn't we lose Python 2 compatibility? I know that there is a comment-based alternative syntax as well, but it looks much more clumsy
what would the performance impact be on very small functions like accessors?

mreineck commented 6 years ago

Sorry, please ignore my second point; I just realized that even with type hints, there will be no type checking at run time...

DanielLenz commented 6 years ago

Thanks for the quick response and feedback!

Yes, the syntax above would only work in python 3. I agree that using the comment-based syntax wouldn't be very elegant, and would also need to be converted at some point in the future. This seems to be the only possibility now, though.

On a related note, are there any plans to drop Python 2 support in a future release (whilst still maintaining a LTS Python 2 version)?

zonca commented 6 years ago

Yes, I think it would be great

zonca commented 6 years ago

As of Python 2, sure, I would follow what astropy does

DanielLenz commented 6 years ago

I've opened an issue (https://github.com/healpy/healpy/issues/468) on the python 3.x only discussion to follow up on that.

As for the type hinting, it makes sense to do it on the python 3.x-only release, mostly because the syntax for mypy is so different from python 2 to 3.

DanielLenz commented 6 years ago

Closing this for now because it requires python>=3, which we won't limit healpy to in the near future. Happy to re-open once we drop python2.7 support.

mlincett commented 1 year ago

Can this issue be reopened now that python 2 is long gone?

zonca commented 1 year ago

Yes, but we do not want to add typing to the current codebase, healpy is in need of a large refactoring to get rid of the current C++ extension and rely on ducc0, which will also allow us to support Windows and simplify the building process. So I think it would be best to wait.