Add typing to utils #139
Add typing to utils #139
Conversation
There was a problem hiding this comment.
Thanks for the PR! I would not be adverse to eventually having full typing support in parfive I think it would make some things easier to reason with.
Apart from the runtime aioftp issue, my comments are mainly my inexperience with typing and my general frustration at what feels like weird work arounds.
parfive/utils.py
Outdated
| from pathlib import Path | ||
| from itertools import count | ||
| from concurrent.futures import ThreadPoolExecutor | ||
|
|
||
| import aioftp |
There was a problem hiding this comment.
aioftp is an optional dependency which currently should only be needed if you try to download a file with an ftp:/ scheme. I think this would break that?
There was a problem hiding this comment.
👍 I've put this import under a TYPE_CHECKING guard so it's only imported when type checking.
parfive/utils.py
Outdated
| @@ -159,47 +162,50 @@ def sha256sum(filename): | |||
|
|
|||
|
|
|||
| class MultiPartDownloadError(Exception): | |||
| def __init__(self, response): | |||
| def __init__(self, response: requests.Response) -> None: | |||
There was a problem hiding this comment.
<rant> requiring you explicitly type -> None on __init__ seems very silly as you can't actually return anything even if you wanted to </rant>
parfive/utils.py
Outdated
| """ | ||
| Given a path generate a unique filename. | ||
| """ | ||
| path = pathlib.Path(path) | ||
| path_ = pathlib.Path(path) |
There was a problem hiding this comment.
🙄 I find this mypy error particularly frustrating. It seems very antithetical to writing clean Python code. Especially here where you are effectively doing runtime type validation.
For my own education: is there a way to persuade mypy to allow explicit type casting like this? or is any change of type an error?
There was a problem hiding this comment.
I think any change in type is an error 😢 . I'll just put a type ignore comment after it, I agree that this is a bit of a pain. Edit: turns out this doesn't matter until/if mypy is set to strict, so I just undid these variable renaming changes.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #139 +/- ##
==========================================
- Coverage 90.23% 90.20% -0.04%
==========================================
Files 5 5
Lines 635 643 +8
==========================================
+ Hits 573 580 +7
- Misses 62 63 +1 ☔ View full report in Codecov by Sentry. |
|
Other than the fact that the mypy ci is angry it looks good. |
Because apparently I type for fun now?