[mypy] Enforce Mypy checks for a subset of modules #208
Conversation
| [mypy-tests.*,trino.auth,trino.client,trino.dbapi,trino.sqlalchemy.*] | ||
| ignore_errors = true |
There was a problem hiding this comment.
Why mypy errors are ignored it these packages? For instance I see that you addressed some in trino.client.py, is it leftover?
There was a problem hiding this comment.
What is mypy-tests.* module? Some leftover?
There was a problem hiding this comment.
@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.
| @@ -456,7 +456,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): | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We should add to the developers setup section in the README how to automatically execute the pre-commit hook when committing as in https://pre-commit.com/#usage
|
@mdesmet @hovaesco I'm fine merging this % #208 (comment). Can you send a follow-up PR with updated instructions in the README.md? |
|
@hashhar : Let's merge this. I will add another PR with updated instructions. |
|
@john-bodley Could you rebase on upstream to resolve conflicts? |
hashhar
force-pushed
the
john-bodley--mypy-enforcement
branch
from
0b958a1
to
1972425
Compare
|
@john-bodley I've rebased to resolve conflicts - no other changes. Once CI finishes and is green we'll merge this. Thanks for doing this work and sorry it took so long to get it done. |
hashhar
added
the
no-release-notes
Even though Mypy is enabled, per this pre-commit hook, the default options are fairly loose which allows for untyped methods and calls.
This PR aims to slowly start tightening the screws module by module (resulting in digestible PRs) —starting with
trino.exceptions,trino.logging, andtrino.transactions—by enforcing typed methods, calls, etc. which improves code clarity and correctness.The hope is over time contributors can systematically go through module by module and ensure all methods are correctly typed and remove them from the ignore list.