Tests: Introduce type linting with mypy #397
Conversation
| ) | ||
|
|
||
|
|
||
| class DryrunTrace: | ||
| def __init__(self, trace: List[dict]): | ||
| self.trace = [DryrunTraceLine(line) for line in trace] | ||
|
|
||
| def get_trace(self) -> List[str]: |
There was a problem hiding this comment.
I removed this method as it is never used and will error if it ever is since the line.trace_line() method does not exist.
| ) | ||
| current_arg = populate_foreign_array( | ||
| current_arg: Any = populate_foreign_array( |
There was a problem hiding this comment.
Ideally this should be a stricter type but it cannot be made any stricter until the parameter type for method_args is constrained to not include Any. To make that argument more specific would likely require specifying types in other places throughout the abi package and is therefore outside the scope of this PR.
There was a problem hiding this comment.
I think this is a good start - agreed we can tackle them outside this PR.
| @@ -2,7 +2,17 @@ | |||
| import copy | |||
| from abc import ABC, abstractmethod | |||
| from enum import IntEnum | |||
| from typing import Any, List, Optional, Tuple, TypeVar, Union | |||
| from typing import ( | |||
There was a problem hiding this comment.
I'd appreciate a closer review of this file because I am not yet very familiar with the ABI types and I found it difficult to determine the types of certain variables from their context.
There was a problem hiding this comment.
Appreciate the effort on adding stricter typing here - this will reduce a lot of maintenance burden 👏
I'm still looking over the PR but here are my immediate comments to your questions. I also opened up a child PR here for reference: jdtzmn#1
There was a problem hiding this comment.
The typing suggestions look good! Part 1 of my review is shown in this child PR - feel free to cherry pick or close and make a new commit as you see fit.
I would also suggest adding a mypy config file (mypy.ini) to the root. If not specified, mypy seems to use a config from the user's local directory, which might be inconsistent with the one that the CI uses. I don't have a preference to which particular set of configs we use, so we can start off with a small set or something that you think is appropriate.
| ) | ||
| current_arg = populate_foreign_array( | ||
| current_arg: Any = populate_foreign_array( |
There was a problem hiding this comment.
I think this is a good start - agreed we can tackle them outside this PR.
|
I have confirmed locally that the types are included when installing from the |
Some types (such as `list` instead of `List`) were introduced in PEP 585 within Python 3.9 but since `py-algorand-sdk`requires Python 3.8 or later, the explicit types (`List`, `Union`, etc) are necessary for compatibility.
This avoids mypy’s fallback behavior which uses the config located in the user’s home directory.
* Add return type to TransactionSigner.sign_transactions * Switch back to List * Fix docstring typo
# Conflicts: # algosdk/atomic_transaction_composer.py
There was a problem hiding this comment.
@jdtzmn Thanks for your efforts here to defend against various classes of error with mypy! ☕
Via inspection of python setup.py sdist | less, I confirmed source distribution contains the mypy stubs as intended.
|
Merging - We can extend mypy to examples + tests in a subsequent PR. |
This PR adds type linting to the SDK using mypy and sets it up as an additional check during CI. A large part of the work here involved fixing existing, previously uncaught errors within the SDK. For a full list of those errors, please refer to the dropdown below.
Full list of resolved errors
Testing
py.typedmarker works correctly.