-
Notifications
You must be signed in to change notification settings - Fork 99
Open
Labels
discussionissues that still need to be discussedissues that still need to be discussedeffort: highMore than 40h total workMore than 40h total workimpact: lowImproving code/documentation cleanliness/clarity, not functionImproving code/documentation cleanliness/clarity, not functionpriority: lowissues that are not urgentissues that are not urgentrefactoringissues proposing/requesting changes to the code which do not impact behaviorissues proposing/requesting changes to the code which do not impact behavior
Description
Summary
Should we start using type hints, when possible, in our functions?
I believe that it would reduce the amount of argument validation that we do at the beginning of each function. Plus it would make our code more transparent.
One drawback is that, in order to support type hints for numpy arrays (one of our most common argument types), we would probably need an additional dependency like nptyping
.
Yea: 👍
Nay: 👎
Additional Detail
@jbteves has started using type hints in #692, which I quite like. He also brought up style guidelines in this month's developers meeting (see #701), so I thought I'd get the ball rolling with this issue.
Next Steps
- Discuss (feel free to weigh in with a vote, but please explain any nays)
- Possibly implement
tsalo, eurunuela, jbteves and handwerkerdemdupre
Metadata
Metadata
Assignees
Labels
discussionissues that still need to be discussedissues that still need to be discussedeffort: highMore than 40h total workMore than 40h total workimpact: lowImproving code/documentation cleanliness/clarity, not functionImproving code/documentation cleanliness/clarity, not functionpriority: lowissues that are not urgentissues that are not urgentrefactoringissues proposing/requesting changes to the code which do not impact behaviorissues proposing/requesting changes to the code which do not impact behavior
Type
Projects
Milestone
Relationships
Development
Select code repository
Activity
jbteves commentedon Mar 22, 2021
Just FYI, the types aren't actually checked at runtime unless we add
mypy
as well. That's why you had to correct one of my type hints in that PR; if they were checked then I would have been getting runtime errors. We can add it if we think it's worth it, however. I included it in that PR mostly to improve documentation and also because frankly I've been using type hints in a different project and forgot we're not using them here 😆 . My view is that we should begin adding type hints in PRs as we modify functions, not do them in one big PR, and addnptyping
as a dependency to make that possible. I need to experiment a bit withmypy
andnpytyping
to decide if they're reliable enough to add type-checking to the whole codebase from my point of view.Thanks for opening @tsalo !
tsalo commentedon Mar 22, 2021
I did not know that. That's somewhat depressing and simultaneously pretty cool!
My concern about this approach (along with the same approach to reformatting with
black
orisort
) is that there are a number of functions that are very stable, and we may not touch them (and thus get them up to snuff) for a long time. If we want to maintain a coding standard across the package, at some point we'll need to bite the bullet and apply our rules to all of the code, or else we'll eventually have a bit of a Frankenstein's monster situation.EDIT: Another relevant task is #448, which could be done piecemeal, but hopefully won't be.
jbteves commentedon Mar 22, 2021
Intriguingly that's my relationship with Python in a nutshell!
I think that's fair. That said, I think that we have enough things going on that adding a major refactor of style is not up at the top of my list. If other developers feel differently, that's worth a discussion, however.
handwerkerd commentedon Mar 22, 2021
I may be optimistic, but, IF types are added to the BIDS outputs branch #691, then modularize metrics #591, and then decision tree modularization #592, we'll have covered a substantial portion of the code base. At that point, systematic addition of types to the rest of the code will be annoying, but should be do-able.
emdupre commentedon Mar 23, 2021
To clarify my 👎 :
I'm for adding type-hints in general -- in that I think they're nice to read -- but I'm against adding dependencies to enforce / check the types. It feels a bit non-pythonic, somehow :)
notZaki commentedon Mar 23, 2021
Would those additional dependencies have to be general dependencies that all users have to install/import or will they be test dependencies? (... and does that matter in swaying opinions?)
jbteves commentedon Mar 23, 2021
It's pretty tricky to make them dependencies for only "some" users, though pretty straightforward to make them dependencies for developers only. I personally think that's a recipe for confusion and should be avoided, however.
tsalo commentedon Mar 25, 2021
I think the main potential benefits would be (1) automatic type checking and (2) automatic type documentation, right? @emdupre I personally like the idea of automatic type checking, but I'm fine not using it. Unfortunately, as far as (2), it doesn't seem like
napoleon
ornumpydoc
support type hints automatically, and the Sphinx extensions that support it don't work with numpydoc-style docstrings yet. All of which is to say that if we don't want to actually validate parameter types, I don't know how much type hints will get us with our current documentation structure. 😞jbteves commentedon Mar 25, 2021
I am obviously biased but (1) but it helps me when doing things on CLI and (2) coming from strongly typed languages it makes things a little more familiar to me. I doubt (2) is as much of a consideration for others, though. Other users may be using servers where it's difficult to set up some sort of IDE, however, and in that case it's nice to have the hints inline with the parameters.
tsalo commentedon Jul 12, 2021
The
nilearn
dev team recently discussed type hints as well. The consensus was (1) the primary benefit is for users/devs using code editors like VSCode, rather than any kind of automatic input validation or documentation, and (2) they would encourage, but not require, type hints in new contributions, but type hints would not be feasible/interpretable for some parts of the library.Of course, we would still need to add
nptyping
to include type hints for most of our parameters, and I don't think we could make it a dev-only requirement unless we did aduecredit
-like thing and included a dummy module to handle ImportErrors automatically...EDIT: Also, given the rather limited impact this would have (short of using
mypy
and figuring out how to supportnumpydoc
withnapoleon
's type hinting extensions), I'm going to downgrade the impact label for this issue.12 remaining items