[Core] Document all exported core methods with type hints #359
Conversation
|
Unless we are going to include a type checker, I am not sure the value here? Also the type is included in the docstring and there should be only one source of truth |
|
Hmm, that's a good point and I hadn't fully thought it out. Here were my goals:
That being said, point 1 is probably not so applicable to the exported methods, because they normally get used via client, or via component.get(), both of which the IDE loses track of the type hints. I still think the type hints are quite useful in the IDE for autocompletion, and I'd be inclined to add them to any bits I touch when doing any fixing/refactoring, unless you are against them completely. For more internal methods the disadvantages above are moot. |
|
I had forgotten about IDE usefulness so happy to try to accommodate there, just didn't want to have the types to become outdated or spend a lot of effort for little gain. Personally I find type hints can reduce code readability and unless constantly encountering type issues not worth the added overhead, especially if never fully verified by mypy. Since we already have a form of typing in the code, I am happy if we experiment with moving types from docstrings. As you suggest starting with core exported methods and use sphinx-autodoc-typehints. |
|
I think that we should also start converting the docstrings to googles's format, probably in another PR. |
|
Can use https://github.com/dadadel/pyment/ to convert them |
|
I did some work in core.py standardizing the docstrings, removing the types, and enabling sphinx generating them from the function signatures. Seems to be working pretty well. |
|
Can we add a mypy check as a github action? We really should have typing everywhere that's feasible. |
Yeah, I was just experimenting with mypy. Trying to figure the easiest way to introduce it without a shitload of comments on stuff to ignore before we get it all in. |
There was a problem hiding this comment.
I haven't used mypy a lot, but I think it could be a good idea to enable it. My only hesitation would be if we start having to add a lot of extra cruft to make mypy understand stuff. (by cruft, I mean stuff that is useless to a human reader, not just making things slightly more verbose, which could be useful to humans) I added a few notes on stuff I changed to get mypy to pass on core.py. My biggest concern is how it complains about twisted reactor. If there was some way we could solve that for the whole project at once it would be great.
deluge/core/core.py
Outdated
| @@ -673,7 +679,7 @@ def pause_torrent(self, torrent_id: str) -> None: | |||
| @export | |||
| def pause_torrents(self, torrent_ids: List[str] = None) -> None: | |||
| """Pauses a list of torrents""" | |||
| if not torrent_ids: | |||
| if torrent_ids is None: | |||
There was a problem hiding this comment.
This change was made so that mypy type narrowing worked and knew that torrent_ids couldn't be None any longer. For some reason not torrent_ids doesn't have the same result as far as mypy is concerned.
There was a problem hiding this comment.
You have to make torrent_ids an Optional[List[str]] since the default is None.
https://docs.python.org/3/library/typing.html#typing.Optional
There was a problem hiding this comment.
Optional is implied when the default is none.
There was a problem hiding this comment.
Hmm, I guess current recommendations are to make the Optional explicit, but the default mypy behavior still allows inferring it. https://mypy.readthedocs.io/en/stable/config_file.html#confval-no_implicit_optional
There was a problem hiding this comment.
Maybe it only makes it Optional if you provide no typing?
There was a problem hiding this comment.
It for sure was getting the Optional type. The issue I was actually trying to solve was: Item "None" of "Optional[List[str]]" has no attribute "__iter__" (not iterable). When the if was just if not torrent_ids it didn't narrow the type down to List[str], but when the check changed to if torrent_ids is not None it did narrow the type down, and allow iterating on it without complaint.
There was a problem hiding this comment.
Huh, I guess it's because of the for loop below the conditional. Mypy doesn't know that you're changing the None into a List[str] with the call to get the torrents list. I suppose there are a few ways to change the code to work without reassigning torrent_ids which would likely fix the issue. I also wonder if setting the return value of TorrentManager.get_torrent_list() to List[str] would help?
There was a problem hiding this comment.
I actually figured it out. It wasn't the not torrent_ids that was messing up, it was that I hadn't typed self.torrentmanager, so it didn't actually know the return type when torrent_ids got reassigned. Did some more testing in another branch and got it working fine.
|
Can we leave the mypy to another PR and get this one merged |
Sure can. I'll back that stuff out, was just doing some experiments. |
|
Restored this to just the doc changes and type hints. |
|
@gazpachoking There are breaking compatibility changes with both older Twisted and Python versions in the PR, I have reverted the code from develop: 1089adb Perhaps we concentrate on fixing other areas of the code and Windows release as I don't see the expenditure just now worth it? |
Ahh. Didn't consider older versions of twisted not having type hinting capability. Shouldn't be any issues with older python though, I didn't use the stdlib types as generics, so that should be fine. The twisted issue would also be resolved if we drop python 3.6 support, was questioning that in my mypy PR. #362
Yeah, that's fine. I just prefer adding the type hints as I go fixing other stuff, so it's a real bummer. |
I thought I saw usage but seems I was mistaken
Certainly can consider dropping it for 2.1.x since it would simplify our support 🤔 If you can see a quick way to get typing in without too much overhead certainly update this PR. I just have limited time to investigate any resulting errors I should also see about running a CI that has our minimum requirements and current requirements |
We could do comment type hints, or, probably nicer, just type Deferreds as Deferreds without typing their return value. If we do maybe_coroutine branch that won't be bad, since we don't return a Deferred if the function is wrapped in that. |
|
I think there shouldn't be problems without the use of generic Deferred return types...
Yeah, that would be useful. |
|
Totally forgot type hints could just be strings. Made that change, it's a much better option. |
Remove type hints in docstrings in favor of the ones in method signatures.
Older versions of twisted (<21.7) don't support generic Deferred type hinting, this prevents crashes on those versions.
|
Rebased to resolve the conflicts, should be good again. |
Started adding some type hints. This one does all the exported methods from core.