Skip to content

[mypy] Enforce Mypy checks for a subset of modules #208

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ repos:
- id: "mypy"
name: "Python: types"
additional_dependencies:
- "types-pytz"
- "types-requests"
- "types-all"

- repo: https://github.com/pycqa/isort
rev: 5.6.4
Expand Down
14 changes: 13 additions & 1 deletion setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,20 @@ test=pytest
[flake8]
max-line-length = 120
# W503 raises a warning when there is a line break before a binary operator.
# This is best practice according to PEP 8 and the rule should be ignored.
# This is best practice according to PEP 8 and the rule should be ignored.
#
# https://www.flake8rules.com/rules/W503.html
# https://www.python.org/dev/peps/pep-0008/#should-a-line-break-before-or-after-a-binary-operator
ignore = W503

[mypy]
check_untyped_defs = true
disallow_any_generics = true
disallow_untyped_calls = true
disallow_untyped_defs = true
ignore_missing_imports = true
no_implicit_optional = true
warn_unused_ignores = true

[mypy-tests.*,trino.auth,trino.client,trino.dbapi,trino.sqlalchemy.*]
ignore_errors = true
Comment on lines +22 to +23
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why mypy errors are ignored it these packages? For instance I see that you addressed some in trino.client.py, is it leftover?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hovaesco I added this comment which hopefully explains why the change in trino/client.py is required.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is mypy-tests.* module? Some leftover?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hashhar mypy-tests is not a module, see here for specifics, but per,

Additional sections named [mypy-PATTERN1,PATTERN2,...] may be present, where PATTERN1, PATTERN2, etc., are comma-separated patterns of fully-qualified module names, with some components optionally replaced by the ‘’ character (e.g. foo.bar, foo.bar., foo.*.baz). These sections specify additional flags that only apply to modules whose name matches at least one of the patterns.

the TL;DR is for the tests.*, trino.auth, etc. modules errors are currently being ignored.

4 changes: 2 additions & 2 deletions trino/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,7 @@ def statement_url(self) -> str:
def next_uri(self) -> Optional[str]:
return self._next_uri

def post(self, sql, additional_http_headers=None):
def post(self, sql: str, additional_http_headers: Optional[Dict[str, Any]] = None):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of yet I'm unsure of the return type. This should become clearer when the module has type enforcement. This method (as well as get) are required to have their arguments typed given they're called in trino/transaction.py and we have disallow_untyped_calls = true set.

data = sql.encode("utf-8")
# Deep copy of the http_headers dict since they may be modified for this
# request by the provided additional_http_headers
Expand Down Expand Up @@ -524,7 +524,7 @@ def post(self, sql, additional_http_headers=None):
)
return http_response

def get(self, url):
def get(self, url: str):
return self._get(
url,
headers=self.http_headers,
Expand Down
24 changes: 12 additions & 12 deletions trino/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
This module defines exceptions for Trino operations. It follows the structure
defined in pep-0249.
"""

from typing import Any, Dict, Optional, Tuple

import trino.logging

Expand Down Expand Up @@ -72,44 +72,44 @@ class TrinoDataError(NotSupportedError):


class TrinoQueryError(Error):
def __init__(self, error, query_id=None):
def __init__(self, error: Dict[str, Any], query_id: Optional[str] = None) -> None:
self._error = error
self._query_id = query_id

@property
def error_code(self):
def error_code(self) -> Optional[int]:
return self._error.get("errorCode", None)

@property
def error_name(self):
def error_name(self) -> Optional[str]:
return self._error.get("errorName", None)

@property
def error_type(self):
def error_type(self) -> Optional[str]:
return self._error.get("errorType", None)

@property
def error_exception(self):
def error_exception(self) -> Optional[str]:
return self.failure_info.get("type", None) if self.failure_info else None

@property
def failure_info(self):
def failure_info(self) -> Optional[Dict[str, Any]]:
return self._error.get("failureInfo", None)

@property
def message(self):
def message(self) -> str:
return self._error.get("message", "Trino did not return an error message")

@property
def error_location(self):
def error_location(self) -> Tuple[int, int]:
location = self._error["errorLocation"]
return (location["lineNumber"], location["columnNumber"])

@property
def query_id(self):
def query_id(self) -> Optional[str]:
return self._query_id

def __repr__(self):
def __repr__(self) -> str:
return '{}(type={}, name={}, message="{}", query_id={})'.format(
self.__class__.__name__,
self.error_type,
Expand All @@ -118,7 +118,7 @@ def __repr__(self):
self.query_id,
)

def __str__(self):
def __str__(self) -> str:
return repr(self)


Expand Down
2 changes: 1 addition & 1 deletion trino/logging.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@


# TODO: provide interface to use ``logging.dictConfig``
def get_logger(name, log_level=LEVEL):
def get_logger(name: str, log_level: int = LEVEL) -> logging.Logger:
logger = logging.getLogger(name)
logger.setLevel(log_level)
return logger
12 changes: 6 additions & 6 deletions trino/transaction.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,19 +50,19 @@ def check(cls, level: int) -> int:


class Transaction(object):
def __init__(self, request):
def __init__(self, request: trino.client.TrinoRequest) -> None:
self._request = request
self._id = NO_TRANSACTION

@property
def id(self):
def id(self) -> str:
return self._id

@property
def request(self):
def request(self) -> trino.client.TrinoRequest:
return self._request

def begin(self):
def begin(self) -> None:
response = self._request.post(START_TRANSACTION)
if not response.ok:
raise trino.exceptions.DatabaseError(
Expand All @@ -81,7 +81,7 @@ def begin(self):
self._request.transaction_id = self._id
logger.info("transaction started: %s", self._id)

def commit(self):
def commit(self) -> None:
query = trino.client.TrinoQuery(self._request, COMMIT)
try:
list(query.execute())
Expand All @@ -92,7 +92,7 @@ def commit(self):
self._id = NO_TRANSACTION
self._request.transaction_id = self._id

def rollback(self):
def rollback(self) -> None:
query = trino.client.TrinoQuery(self._request, ROLLBACK)
try:
list(query.execute())
Expand Down