DrMarc / slab

python classes for working with sounds and conducting psychoacoustic experiments
MIT License
22 stars 7 forks source link

Type-hinting of user-facing functions #28

Closed hadware closed 3 years ago

hadware commented 3 years ago

You probably expected this, but here it is:

Your docstrings are pretty exhaustive already (especially regarding expected types), but type hints are really usefull when using a type-aware IDE (pycharm for instance). This shouldn't be too complicated, as most of your inputs are either str, int or float.

A couple of pointers if you've never done this before:

You could just add these type-hints to functions that users are going to use i you feel that it's useless for your internal API.

DrMarc commented 3 years ago

I'm on the fence on type hints for project of this size/complexity and intended audience. Here is the rationale: Type hints have well-known pros and cons. With types already specified in the docstrings, the major remaining pro is machine readability for users that happen to have a type-aware IDE. The major downside is decreased human readability of call signatures when calling help(). Most of our parameters can be one of several types: for numbers often Union[int, float, numpy.int, numpy.float, etc.]; for slab objects often any class with .data and .samplerate attributes. Same for return arguments. This will also require a substantial rewrite, because we specify types in the docstrings. Having two sources of type info is problematic. We'd have to remove docstring types and then auto-generate them from the type hints, which again renders in a fashion that is hard to understand (if one is not familiar with it) and not beginner friendly. I'd like to prioritise humans over machine readability here, because beginners are specifically among the target audience. If both reviewers see this as a 'must' and a JOSS requirement, then we'll do it of course, but for the reasons above I'd rather not.

hadware commented 3 years ago

I agree that type annotation duplication (one in the signature, one in the docstring) is a real pain in the rear. I don't know if there's a docstring style (google, numpy, restructured, etc...) that plays nice with them. My usual take on this just to go with it and have it in both places... not optimal in any way.

Most of our parameters can be one of several types: for numbers often Union[int, float, numpy.int, numpy.float, etc.]

Regarding this issue, I think just making it Number, or alias it to something like MyNumberType = Union[int, float, numpy.int, numpy.float, etc.]

regarding the JOSS submission, I was just suggesting that as an improvement, it's in no way necessary ;)

DrMarc commented 3 years ago

Using Number or aliasing is a good idea. I do agree that it's generally good practice. I'll look into solving the docs issue and will consider moving the type hints into the call strings.