DM-55832: Clean up some type annotations - #154
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #154 +/- ##
==========================================
- Coverage 83.38% 83.31% -0.08%
==========================================
Files 36 36
Lines 7995 8025 +30
Branches 960 972 +12
==========================================
+ Hits 6667 6686 +19
- Misses 1032 1043 +11
Partials 296 296 ☔ View full report in Codecov by Harness. |
13290aa to
6583d78
Compare
| echo "Enable upload" | ||
| echo "skip=false" >> "${GITHUB_OUTPUT}" | ||
| fi | ||
|
|
There was a problem hiding this comment.
I assume the eventual plan is to move this somewhere reusable?
There was a problem hiding this comment.
Yes. It's on @mwittgen 's todo list.... so currently we have to copy it to every pypi repo to make sure we don't upload something to PyPI every weekly.
| *, | ||
| encoding: str | None = None, | ||
| ) -> Generator[IO, None, None]: | ||
| ) -> Generator[ResourceHandleProtocol, None, None]: |
There was a problem hiding this comment.
If ResourceHandleProtocol doesn't implement typing.IO[Any] that might be a real problem. Or at least I'd like to understand what's missing.
Or is this a case where ResourceHandleProtocol has extra methods and the caller might want to use them?
There was a problem hiding this comment.
ResourceHandleProtocol seems to define all the methods you would expect an IO to support without saying it is an IO. Maybe that was intentional but seems like it would be preferable for it to be an IO.
There was a problem hiding this comment.
It seems like:
- A protocol can not inherit from IO
- We could have BaseResourceHandle inherit from IO
- There is a mismatch between
isattyas a property vs a method. - The
readlinessignature is also wrong. - There is no
__iter__. - The
write()methods need to accept memoryview and bytearray and they don't.
The isatty is the one that breaks everything.
@natelust what was your reasoning for not quite having an IO?
There was a problem hiding this comment.
I'm not sure this was it, but I think back when this was written, there was some dispute (or maybe just confusion on our end) about whether typing.IO or io.IOBase (or one of its subclasses) was the annotation we wanted to satisfy.
There was a problem hiding this comment.
Either way isatty() is always a method and not a property.
There was a problem hiding this comment.
We likely should fix all this but not on this ticket.
fae1334 to
f098913
Compare
They don't recognize abstractproperty as being special.
Also fixes the S3 boto.client annotation which was also wrong
The encoding parameter was declared third positionally, where the AbstractFileSystem base class declares block_size. A caller passing block_size positionally would have bound it to encoding instead. Move encoding after the base class parameters and make it keyword-only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ResourcePath.__new__ dispatches on scheme and so is declared as returning the base class. ty follows that declaration and reports subclass attributes as unresolved; mypy resolves the constructor to the subclass and rejects a cast as redundant. A ty-specific suppression is the only form both checkers accept. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The requests stubs declare AuthBase.__call__ with a parameter named r, which callers may pass by keyword, so an override naming it req is not a valid substitute. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
defusedxml.ElementTree does not re-export Element, so eTree.Element was unresolvable for type checkers. defusedxml hardens the parser but builds trees from the standard library element type, so import Element directly for the annotations. Parsing still goes through eTree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Traversable.open is overloaded on Literal["r"] and Literal["rb"], so a variable mode could not select an overload and needed a call-overload suppression. Branch on the mode explicitly instead, which also normalizes modes such as "br" that zipfile.Path rejects but pathlib.Path accepts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The boto3 section only matched the top level module, so importing boto3.s3.transfer still required an inline suppression. Widen it to boto3.* to match the existing botocore.* handling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ResourcePath.open documents encoding as being ignored for binary IO, but FileResourceHandle and the prefer_file_temporary path both forwarded it into the builtin open(), which raises ValueError for a binary mode. Drop it in binary mode, as is already done for newline, so that all schemes agree. Covered by the shared GenericReadWriteTestCase.test_open so every scheme is exercised. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
defusedxml ships no type information, so importing it as eTree left every XML call typed as Any, and the conditional fallback import of the stdlib module was reported as a redefinition by mypy in some environments. Give type checkers the standard library module that defusedxml hardens and mirrors, while the runtime continues to prefer defusedxml when available. This also makes the defusedxml mypy override unnecessary.
backoff, moto and aiohttp all ship a py.typed marker, so their ignore_missing_imports overrides no longer have any effect. The wsgidav override is retained. It is reported as unused only because CI type checks the python directory, and wsgidav is imported solely by the tests.
The fsspec HTTP backend in the http module imports aiohttp directly, but it was only ever installed as a transitive dependency of s3fs during testing and was absent entirely when type checking. List it in the https extra and in requirements.txt so that it is present wherever the code that uses it is exercised or checked.
2 participants
Checklist
doc/changes