search

matthewholman / assist

ASSIST is a software package for ephemeris-quality integrations of test particles.
https://assist.readthedocs.io/
GNU General Public License v3.0
24 stars 10 forks source link

Add type annotation hints to Python modules #80

Closed spenczar closed 1 year ago

spenczar commented 1 year ago

This PR adds type annotations to the Python modules. These are not used at runtime; they're merely advisory notes that can be used by static code analyzers to detect bugs for users of assist.

I thought about making an issue to bring up this idea, but then I figured that it's even quicker to just make a PR with the proposed changes.

I think this would be a mild improvement for sophisticated Python users, as it could get them a bit closer to complete type coverage for programs that use assist.

hannorein commented 1 year ago

Thanks for that. Personally, I'm always on the fence when it comes to type hints. They can clearly be useful, but there are downsides to them too: they make the code longer and, in my opinion, harder to read for people who are not sophisticated Python users as one needs to understand the extra syntax rules (or at least understand how to ignore them). In any case, I think the pros probably outweigh the cons here, so we should go ahead. Thanks also for the various other small fixes.

Right now the unit tests are still failing. Can you make them pass?

spenczar commented 1 year ago

Personally, I'm always on the fence when it comes to type hints.

I agree, actually, and I think a few of the changes in this PR might make the code a bit worse. This PR adds type annotations to the public API, but also is the result of running mypy on the codebase itself to make sure it's internally consistent.

The new _libassist_str(name: str) -> str function is an example of this. mypy complains that c_char_p.in_dll(clibassist, "assist_githash_str").value.decode('ascii') does not address the possibility that c_char_p.in_dll(clibassist, "assist_githash_str").value is None... but I don't think that's really worth worrying about. But just to silence it, I added the new function.

One thing I could do is trim this PR down to just provide type annotations for the public API, but go no further. It makes it a little harder to verify that the public API is completely and correctly typed, but so it goes. What do you think?

Right now the unit tests are still failing. Can you make them pass?

Yes, of course, sorry - I didn't notice the Python 3.8 support.

hannorein commented 1 year ago

Ah, I see - that explains the changes you made! I think they are fine. No need to take anything out.

hannorein commented 1 year ago

Thanks! Tests are passing. @matthewholman any other thoughts or are we ready to merge?

matthewholman commented 1 year ago

I think it's ready to merge. I don't have a strong opinion.