banteg / multicall.py

aggregate results of multiple smart contract calls into one
MIT License
246 stars 106 forks source link

Fixing typehinting in Call's `returns` argument #64

Open DefiDebauchery opened 1 year ago

DefiDebauchery commented 1 year ago

I'm not strongly-versed in python typing (especially in pre-3.9 versions where hints became much more intuitive), otherwise I would have sent a PR directly, but trying to understand.

Call's __init__ method has a argument returns (and deocde_output()), which is defined as such

class Call:
  def __init__(
    ...
    returns: Optional[Iterable[Tuple[str,Callable]]] = None,
    ...
  ) -> None:

Since the second argument to the list can be None, would it be better for the Callable to be replaced with Optional[Callable] or simply Callable | None (or I guess in <3.9 syntax, Union[Callable, None])?

I have never seen the use of Tuples anywhere - both the examples and the tests use lists - but changing Tuple to List throws its own error since it's not expecting more than one argument to the List type. Changing it to Iterable[str, Optional[Callable]], while passing lint check, seems wonky because it definitely doesn't accept dicts.

What's the best way to properly represent the inputs? This is relatively minor in the grand scope of things, but is an annoyance during development, and being technically correct is the best kind of correct.

BobTheBuidler commented 1 year ago

"but changing Tuple to List throws its own error since it's not expecting more than one argument to the List type."

This is the reason it says Tuple instead of List, but for your typechecker it should be good enough

"Since the second argument to the list can be None, would it be better for the Callable to be replaced with Optional[Callable]"

This would be fine

"while passing lint check, seems wonky because it definitely doesn't accept dicts."

I guess it might accept a dict if dict.keys() are the args, but that's weird :P Iterable seems most technically correct to me, I wouldn't be opposed to using that.