Skip to content
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

Fixing typehinting in Call's returns argument #64

Open
DefiDebauchery opened this issue Feb 18, 2023 · 1 comment
Open

Fixing typehinting in Call's returns argument #64

DefiDebauchery opened this issue Feb 18, 2023 · 1 comment

Comments

@DefiDebauchery
Copy link
Contributor

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
Copy link
Collaborator

"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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants