Add type hints and enable mypy for client.py and dbapi.py by damian3031 · Pull Request #324 · trinodb/trino-python-client

damian3031 · 2023-01-18T13:26:46Z

part of #292

Added type hints and enabled mypy checks for client.py, dbapi.py

trino/dbapi.py

mdesmet · 2023-01-18T15:39:05Z

trino/dbapi.py


-    def genall(self):
-        return self._query.result
+    def genall(self) -> Any:


Suggested change

def genall(self) -> Any:

def genall(self) -> Optional[trino.client.TrinoResult]:

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

trino/exceptions.py

trino/client.py

mdesmet

LGTM % the Iterator comment

trino/dbapi.py

hashhar

first quick pass

hashhar · 2023-01-19T07:01:36Z

trino/client.py

    PROXIES = {}

-_HEADER_EXTRA_CREDENTIAL_KEY_REGEX = re.compile(r'^\S[^\s=]*$')
+_HEADER_EXTRA_CREDENTIAL_KEY_REGEX = re.compile(r"^\S[^\s=]*$")


separate out a commit, having functional and non-functional changes in single commit makes it harder to read history (bisecting for example) in future.

Extracted to separate commit

trino/client.py

hashhar · 2023-01-19T07:12:15Z

trino/dbapi.py

-    def commit(self):
-        if self.transaction is None:
+    def commit(self) -> None:
+        if self._transaction is None:


why go directly to field?

Used transaction property, instead of directly _transaction attribute

hashhar · 2023-01-19T07:13:04Z

trino/dbapi.py

        )

-    def cursor(self, legacy_primitive_types: bool = None):
+    def cursor(self, legacy_primitive_types: Optional[bool] = None) -> 'Cursor':


-> 'Cursor'? Why no qualify the name?

here and other places where we use "strings" as return types

Fixed to use Postponed Evaluation of Annotations when type hint contains names that have not been defined yet.

hashhar · 2023-01-19T07:14:19Z

trino/dbapi.py

        for parameters in seq_of_params[:-1]:
            self.execute(operation, parameters)
            self.fetchall()
+            assert self._query is not None


potential bugfix, extract a commit

extracted to separate commit

hashhar · 2023-01-19T07:14:41Z

trino/dbapi.py

+        if self._query:
+            return self._query.result
+        return None


potential bugfix, extract a commit

extracted to separate commit

trino/dbapi.py

damian3031 · 2023-01-20T16:16:30Z

@hashhar I've adjusted to all your comments, PTAL

cla-bot bot added the cla-signed label Jan 18, 2023

damian3031 requested review from hashhar, hovaesco and mdesmet and removed request for mdesmet January 18, 2023 13:26

mdesmet reviewed Jan 18, 2023

View reviewed changes

damian3031 force-pushed the hovaesco/mypy2 branch from 57c9706 to f58375e Compare January 18, 2023 16:43

mdesmet reviewed Jan 18, 2023

View reviewed changes

trino/client.py Outdated Show resolved Hide resolved

damian3031 force-pushed the hovaesco/mypy2 branch from 7998fb7 to 4b6e2e3 Compare January 18, 2023 19:48

mdesmet reviewed Jan 18, 2023

View reviewed changes

trino/dbapi.py Outdated Show resolved Hide resolved

hashhar changed the title ~~Hovaesco/mypy2~~ Add type hints and enable mypy for client.py and dbapi.py Jan 19, 2023

hashhar reviewed Jan 19, 2023

View reviewed changes

damian3031 force-pushed the hovaesco/mypy2 branch 2 times, most recently from d906660 to e4e8bce Compare January 20, 2023 09:23

damian3031 added 3 commits January 20, 2023 11:53

mypy checks

97b7773

Small fixes

a45c000

Reformatting

435a30f

damian3031 force-pushed the hovaesco/mypy2 branch 2 times, most recently from 96998ec to edc9e9a Compare January 20, 2023 14:35

damian3031 added 2 commits January 20, 2023 16:47

Enable mypy checks for client.py

02be191

Small fix in trino.exceptions.TrinoQueryError

3de613f

damian3031 force-pushed the hovaesco/mypy2 branch from edc9e9a to 3de613f Compare January 20, 2023 15:48

hovaesco assigned damian3031 Mar 30, 2023

	def genall(self) -> Any:
	def genall(self) -> Optional[trino.client.TrinoResult]:

Comments

Conversation

damian3031 commented Jan 18, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mdesmet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

damian3031 commented Jan 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants