Add type hints and enable mypy for client.py and dbapi.py #324
Conversation
damian3031
requested review from
mdesmet,
hovaesco and
hashhar
and removed request for
mdesmet
| @@ -584,66 +603,22 @@ def describe(self, sql: str) -> List[DescribeOutput]: | |||
|
|
|||
| return list(map(lambda x: DescribeOutput.from_row(x), result)) | |||
|
|
|||
| def genall(self): | |||
| return self._query.result | |||
| def genall(self) -> Any: | |||
There was a problem hiding this comment.
| def genall(self) -> Any: | |
| def genall(self) -> Optional[trino.client.TrinoResult]: |
There was a problem hiding this comment.
mypy throws error then:
trino/dbapi.py:615: error: Argument 1 to "list" has incompatible type "Optional[TrinoResult]"; expected "Iterable[List[Any]]" [arg-type]
changing to Iterable[List[Any]] throws error elsewhere, and so on
damian3031
force-pushed
the
hovaesco/mypy2
branch
from
57c9706
to
f58375e
Compare
damian3031
force-pushed
the
hovaesco/mypy2
branch
from
7998fb7
to
4b6e2e3
Compare
hashhar
changed the title
| @@ -75,7 +87,7 @@ | |||
| else: | |||
| PROXIES = {} | |||
|
|
|||
| _HEADER_EXTRA_CREDENTIAL_KEY_REGEX = re.compile(r'^\S[^\s=]*$') | |||
| _HEADER_EXTRA_CREDENTIAL_KEY_REGEX = re.compile(r"^\S[^\s=]*$") | |||
There was a problem hiding this comment.
separate out a commit, having functional and non-functional changes in single commit makes it harder to read history (bisecting for example) in future.
There was a problem hiding this comment.
Extracted to separate commit
trino/dbapi.py
Outdated
| def commit(self): | ||
| if self.transaction is None: | ||
| def commit(self) -> None: | ||
| if self._transaction is None: |
There was a problem hiding this comment.
Used transaction property, instead of directly _transaction attribute
trino/dbapi.py
Outdated
| @@ -207,7 +212,7 @@ def _create_request(self): | |||
| self.request_timeout, | |||
| ) | |||
|
|
|||
| def cursor(self, legacy_primitive_types: bool = None): | |||
| def cursor(self, legacy_primitive_types: Optional[bool] = None) -> 'Cursor': | |||
There was a problem hiding this comment.
-> 'Cursor'? Why no qualify the name?
There was a problem hiding this comment.
here and other places where we use "strings" as return types
There was a problem hiding this comment.
Fixed to use Postponed Evaluation of Annotations when type hint contains names that have not been defined yet.
| @@ -503,6 +521,7 @@ def executemany(self, operation, seq_of_params): | |||
| for parameters in seq_of_params[:-1]: | |||
| self.execute(operation, parameters) | |||
| self.fetchall() | |||
| assert self._query is not None | |||
There was a problem hiding this comment.
potential bugfix, extract a commit
There was a problem hiding this comment.
extracted to separate commit
| if self._query: | ||
| return self._query.result | ||
| return None |
There was a problem hiding this comment.
potential bugfix, extract a commit
There was a problem hiding this comment.
extracted to separate commit
damian3031
force-pushed
the
hovaesco/mypy2
branch
2 times, most recently
from
d906660
to
e4e8bce
Compare
damian3031
force-pushed
the
hovaesco/mypy2
branch
2 times, most recently
from
96998ec
to
edc9e9a
Compare
damian3031
force-pushed
the
hovaesco/mypy2
branch
from
edc9e9a
to
3de613f
Compare
|
@hashhar I've adjusted to all your comments, PTAL |
part of #292
Added type hints and enabled mypy checks for
client.py,dbapi.py