-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add type hints to web3.providers.ethtester #1518
Add type hints to web3.providers.ethtester #1518
Conversation
9648037
to
48fcf7a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to re-review once you've made another pass at tightening some of the types (assuming they can actually be tightened)
@@ -190,7 +191,7 @@ def address_in(address: ChecksumAddress, addresses: Collection[ChecksumAddress]) | |||
|
|||
|
|||
def address_to_reverse_domain(address: ChecksumAddress) -> str: | |||
lower_unprefixed_address = remove_0x_prefix(to_normalized_address(address)) | |||
lower_unprefixed_address = remove_0x_prefix(HexStr(to_normalized_address(address))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR but it would be ideal if we got our typing right so that we didn't need to wrap this in HexStr
.
if fn_kwargs is None: | ||
fn_kwargs = {} | ||
return getattr(eth_tester, fn_name)(*fn_args, **fn_kwargs) | ||
|
||
|
||
def without_eth_tester(fn): | ||
def without_eth_tester(fn: Callable[..., Any]) -> Callable[..., RPCResponse]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be more accurately typed as:
TReturn = TypeVar("TReturn")
TParams = TypeVar("TParams")
def without_eth_tester(fn: Callable[[TParams], TReturn]) -> Callable[[EThereumTester, TParams], TReturn]:
...
return fn(params) | ||
return inner | ||
|
||
|
||
def without_params(fn): | ||
def without_params(fn: Callable[..., Any]) -> Callable[..., RPCResponse]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Typing can be more precicesly defined using TypeVar
types.
return eth_tester, preprocessor_fn(params) | ||
|
||
|
||
def static_return(value): | ||
def inner(*args, **kwargs): | ||
def static_return(value: Any) -> Callable[..., Any]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one should be doable as def static_return(value: TValue) -> Callable[..., TValue]:
@@ -73,7 +101,7 @@ def client_version(eth_tester, params): | |||
|
|||
|
|||
@curry | |||
def null_if_excepts(exc_type, fn): | |||
def null_if_excepts(exc_type: Type[BaseException], fn: Callable[..., Any]) -> Callable[..., Any]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, types can be tightened using TypeVar
for callable return type.
filter_params = params[0] | ||
filter_id = eth_tester.create_log_filter(**filter_params) | ||
return filter_id | ||
|
||
|
||
def get_logs(eth_tester, params): | ||
def get_logs(eth_tester: "EthereumTester", params: Any) -> List[LogReceipt]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For each of these that is a defined API we should know what the params
value looks like and thus be able to use stricture types than Any
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Will take care of tightening up the params value in a different pr to avoid bloat.
return eth_tester.add_account(_generate_random_private_key()) | ||
|
||
|
||
def personal_send_transaction(eth_tester, params): | ||
def personal_send_transaction(eth_tester: "EthereumTester", params: Any) -> HexStr: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
params here should be someting like Optional[TxParams]
I think.
async def make_request(self, method, params): | ||
async def make_request( | ||
self, method: RPCEndpoint, params: Any | ||
) -> Coroutine[Any, Any, RPCResponse]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by this returning a Coroutine
type. I would have expected just RPCResponse
...
@@ -55,7 +81,7 @@ def __init__(self, ethereum_tester=None, api_endpoints=None): | |||
else: | |||
self.api_endpoints = api_endpoints | |||
|
|||
def make_request(self, method, params): | |||
def make_request(self, method: RPCEndpoint, params: Any) -> RPCResponse: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At minimum do we know that params is a List[Any]
? I'm not 100% sure but I think that we have some knowledge that params is a list of values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
params
This will be a list or other iterable of the parameters for the JSON-RPC method being called.
Yup! - but it from some testing it seems that it requires a List
type not Sequence
/ Iterable
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making this update touches a lot of files, so to avoid from bloating this pr, i'll follow it up with another that tightens this up
48fcf7a
to
e043637
Compare
What was wrong?
Type hints missing from
web3.providers.ethtester
Cute Animal Picture