refactor(dbapi): add type annotations to pass mypy (part of #692)#800
refactor(dbapi): add type annotations to pass mypy (part of #692)#800rikettsie wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR continues the effort in #692 by adding/adjusting type annotations in the clickhouse_connect.dbapi layer so it passes mypy cleanly, and then removing the DB-API modules from the mypy ignore_errors baseline in pyproject.toml.
Changes:
- Remove
clickhouse_connect.dbapi*modules from the mypy ignore baseline. - Add/adjust type annotations for DB-API
connect(),Connection, andCursormethods/properties. - Minor refactors in
Cursoraround bulk insert handling (e.g.,op_columnsnormalization) to satisfy typing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| pyproject.toml | Removes DB-API modules from the mypy ignore baseline so they must type-check cleanly. |
| clickhouse_connect/dbapi/cursor.py | Adds method/property annotations and refines some insert-path typing logic. |
| clickhouse_connect/dbapi/connection.py | Annotates DB-API connection wrapper and normalizes args passed to create_client. |
| clickhouse_connect/dbapi/init.py | Annotates the DB-API connect() entrypoint and kwargs typing. |
- Port annotation 'int | None = 0' inconsistency; - Use explicit 'is None' check instead of 'or' to avoid silently treating an empty string as __default__; - Change _summary type from 'dict[str, str]' to 'dict[str, Any]'; - Ensure _try_bulk_insert returns resetting _rowcount and _ix.
|
@mshustov, I addressed all Copilot feedback. A couple of comments were genuine fixes. The others were false alarms as I explained in the resolution threads, specifically around |
|
@joe-clickhouse, @peter-leonov-ch, @mshustov: the CI failure is a network timeout pulling the ClickHouse Docker image from Docker Hub, not related to the changes. Could you re-run it? |
|
@rikettsie awesome work so far! The annotations look good and I agree with your reasoning on the port and database sentinels. I took a look and also confirmed both Copilot's objections there are false alarms. Two requests before merge though. The bulk-insert path now updates |
Summary
Annotate the
dbapilayer (initialization, connection, cursor) so it passes mypy cleanly and can be removed from the ignore_errors baseline in pyproject.toml. This is part of issue #692.Some non-obvious choices I made:
On
Cursor, I declaredself.namesandself.typesasSequence(notlist) since they can be bothtuple(fromquery_result.column_names) and list (from the comprehensions a couple of lines below).I kept
check_valid()the void guard it was, but enforced the use ofcast(Sequence, self.data)in callers (e.g.fetchall, ...) to inform mypy the value is neverNoneafter that point. That's a zero-cost type hint, another option could be using an assert.I annotaed
usernameandpasswordinconnect(), withstr(notstr | None) since their defaults were"". By contrast,Nonecarries no distinct meaning here, unlikehost,database, andinterfacewhich can legitimately be absent.In
connection.py, I annotated the return type ofcommand()asAnybecause its type is unknown until we annotate the driver layer in a follow-up PRs.In all the other points where I used
Any, the data type is genuinely polymorphic (in my understanding of the possibilities).Checklist