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

Revert "get type from function annotation" #209

Closed
sobolevn opened this issue Jun 7, 2021 · 0 comments · Fixed by #207
Closed

Revert "get type from function annotation" #209

sobolevn opened this issue Jun 7, 2021 · 0 comments · Fixed by #207
Assignees

Comments

@sobolevn
Copy link
Member

sobolevn commented Jun 7, 2021

Ok, I really thought that this is a good idea. But, now it seems to me that I was wrong.

Why?

  1. First of all, it is very limited. For example, you cannot write def a(instance: Union[str, bytes]). We can only work with Instance types because of the limited runtime-dispatch capabilities
  2. It makes our code much more complex. And I mean it! With this feature we now have to typecheck a whole other set of cases. Let's see some examples:
def ex():
   ...

some.instance(ex)  # will typecheck, because we allow callables, but should not.
# So, we need a plugin support for this - and it is really complex, because it has lots of corner cases
  1. It makes our code much less type-safe. There are cases when Callable should not be allowed into first .instance() call
  2. Runtime algorithm is not perfect as well, it is now focused on getattr(arg, '__annotations__') and the thing is arg might have __annotations__ even if it is not a function. So, I need to figure out another solution. But, probably the best way to solve this is to remove this feature

So, I have decided to remove this. Yes, it is a breaking changed, but there are just several users, and upgrade is really easy.

Reverts #136

@sobolevn sobolevn self-assigned this Jun 7, 2021
sobolevn added a commit that referenced this issue Jun 7, 2021
sobolevn added a commit that referenced this issue Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

1 participant