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

Support isinstance() checks in the base types #828

Merged
merged 2 commits into from Apr 12, 2022

Conversation

vmarkovtsev
Copy link
Contributor

To avoid the error on e.g. isinstance(QueryType(), SchemaBindable)

TypeError: Instance and class checks can only be used with @runtime_checkable protocols

ariadne/types.py Outdated Show resolved Hide resolved
@rafalp
Copy link
Contributor

rafalp commented Apr 12, 2022

@vmarkovtsev we also need to to move extension classes to protocol.

@vmarkovtsev
Copy link
Contributor Author

@rafalp why do we need to move classes in the same PR? Or I don't get what you request...

@rafalp
Copy link
Contributor

rafalp commented Apr 12, 2022

@vmarkovtsev can you rebase against master so tests pipeline is run? You will see an error then that @runtime_checkable can only be used on classes that extend Protocol which is not the case for Extensions.

vmarkovtsev and others added 2 commits April 12, 2022 13:49
To avoid the error on e.g. `isinstance(QueryType(), SchemaBindable)`

```
TypeError: Instance and class checks can only be used with @runtime_checkable protocols
```
Co-authored-by: Rafał Pitoń <kontakt@rpiton.com>
@vmarkovtsev
Copy link
Contributor Author

vmarkovtsev commented Apr 12, 2022

I've rebased, however, there are two things:

  1. Extension does inherit from Protocol. In fact, I have this PR already working in our production, so I wonder how come something's broken.
    image
  2. CI does not trigger for first-time contributors like me and has to be approved manually, I think.

@rafalp
Copy link
Contributor

rafalp commented Apr 12, 2022

Okay, so GH is not letting me run CI for this PR because for some reason there's no "Approve and Run" option 🤦‍♂️

@rafalp
Copy link
Contributor

rafalp commented Apr 12, 2022

Okay, looking good. Thank you!

@rafalp rafalp merged commit 64cf7c4 into mirumee:master Apr 12, 2022
@rafalp
Copy link
Contributor

rafalp commented Apr 12, 2022

@vmarkovtsev vmarkovtsev deleted the patch-1 branch April 12, 2022 12:57
@vmarkovtsev
Copy link
Contributor Author

LOL. I'll try to PR a fix ASAP!

@rafalp
Copy link
Contributor

rafalp commented Apr 12, 2022

@vmarkovtsev I've removed decorator from one extension class for now and I am researching why GitHub is not running actions for forks. Looks like PR authors need to enable actions on their fork first?

@rafalp
Copy link
Contributor

rafalp commented Apr 12, 2022

I'll release current master as 0.15.0dev5 so you can ride the edge if you want to.

@vmarkovtsev
Copy link
Contributor Author

So

@runtime_checkable
class Extension(Protocol)

works but

@runtime_checkable
class ExtensionSync(Extension):

doesn't 🤔 Maybe it's not needed because the parent class is already runtime_checkable and the reported error is misleading 🤔

@vmarkovtsev
Copy link
Contributor Author

Thanks @rafalp!

@rafalp
Copy link
Contributor

rafalp commented Apr 12, 2022

0.15.0.dev5 is out.

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

Successfully merging this pull request may close these issues.

None yet

2 participants