-
Notifications
You must be signed in to change notification settings - Fork 121
Add type hints to all public methods, functions and classes #462
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
Conversation
Add type hints to all public methods, functions and classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I am very conflicted about this PR. Type hints add so much noise and complexity to the code base, without providing too much benefit to the way I like to code. However, much though I'm not a fan, I recognize that type hints are a modern feature of Python that some programmers rely on. But I'd ask you to find a bit of a compromise between verbosity and completeness.
In particular, the very fine grained literals and overloads are too much. They add dozens of lines of code, and duplication, which is a maintenance burden I am not willing to carry.
I also don't like the casting and reformatting, which seem unnecessary, and do not make the code clearer.
If you see these things differently, I'm open to discuss them. But please offer concrete examples of how these features improve particular workflows if you deem them necessary.
First of all, thank you very much for reviewing my PR. As shown in the PR, I frequently use type hints in Python and have built many systems this way. In this PR, I tried to make the typing as complete as possible. However, as you pointed out, detailed literals and overloads can make the program excessively complex. I'm happy to follow the maintainer's wishes, and I can certainly implement simpler type hints with fewer changes by avoiding detailed literals and overloads. Specifically, I could change literals to just str and remove all overloads in favor of Unions. (Though this would mean the dtype of NumPy NDArray return values in read and blocks functions would become Any). For reference, the benefit of using literals is that they allow validation of possible string patterns before executing the code. Overloads also enable more precise inference by type checkers, making this library easier to use in more robust systems. However, I also recognize this might be overengineering for what is essentially a simple Python wrapper for libsndfile. What format would you prefer for the final version? I'm very happy to be able to contribute to soundfile. |
Let's try to make the type hints as minimal as possible. Perhaps you can generate the literals from the existing dictionaries? I definitely don't want the list of keys duplicated in the dicts and the literals. |
Thank you for providing direction. I'll work to minimize duplication as much as possible. However, Literals need to explicitly enumerate all possible values when they are defined. Therefore, generating them dynamically from dictionaries is difficult from a type system perspective. |
@bastibe Work done! |
Thank you very much. I'm much happier with the current state of the code. I read up on the spaces around default argument assignments with type hints, and found your initial commit to be correct. My apologies for that. You can revert to putting spaces around default argument assignments if you want. Regarding your comments on a formatter, I have found them too restrictive in the past. This may have changed in the meantime, however, and I would love to hear your perspective on the matter. I would also be interested in your opinion on the current state of this pull request. Does it still help your coding, or have we removed too much? Regardless, this now looks good to me, and can be merged if you deem it ready. |
Thank you for checking. I will revert the spacing issue. Regarding formatters, I consider them very important for maintaining clean code when multiple people are developing a codebase. Using ruff would be a good choice nowadays. As for the current state of the PR, I think it's fine. I've confirmed that functions like |
This reverts commit 29c215f.
@bastibe Ready! |
Thank you very much for your contribution! |
Hi @bastibe, Do you plan on publishing a new release of soundfile that includes these changes? I just added mypy to a project that uses soundfile and the type hints would help lightening it. Thanks a lot! |
A new release of soundfile is in the works, but my spare time for this is limited at the moment. |
I've added Python type hints to the
soundfile.py
! All public methods, functions, and classes now have type annotations to support static type checking with pyright.Key improvements:
This implements the type hints requested in Issue #461. The changes enable better IDE autocompletion and error detection through static type checkers like pyright.
Looking forward to your review!