Skip to content
This repository was archived by the owner on Sep 6, 2022. It is now read-only.

Conversation

Emsu
Copy link
Contributor

@Emsu Emsu commented Sep 24, 2018

Note:
When using graphene Interfaces, previous implementation could
display false positives by only checking if the root type matches
with the given model. Now if you pass in an Interface type, the
check will check that the object type's class hierarchy is a subset of
the instance's class hierarchy

Note:
When using graphene Interfaces, previous implementation could
display false positives by only checking if the root type matches
with the given model. Now if you pass in an Interface type, the
check will check that the object type's class hierarchy is a subset of
the instance's class hierarchy
@Emsu
Copy link
Contributor Author

Emsu commented Sep 24, 2018

As an example, if you had the following models:

Notification (ndb.PolyModel)
|-JobNotification
|-ApplicationNotification

where JobNotification and ApplicationNotification where subclasses of Notification, and you checked if an instance of ApplicationNotification(ndb.Model) was a valid model for a NdbObjectType of JobNotification, it would return true instead of false. (It Checks that Notification is in [Notification, JobNotification] instead of if [Notification, ApplicationNotification], is in [Notification, JobNotification]).

The language I'm using might be a little confusing so let me know if you have any questions.

@ekampf
Copy link
Collaborator

ekampf commented Sep 26, 2018

@Emsu thanks for the fix but some tests are failing... can you fix so I can merge?

@coveralls
Copy link

coveralls commented Sep 27, 2018

Coverage Status

Coverage remained the same at 91.608% when pulling 65549d8 on Emsu:master into 751ab0f on graphql-python:master.

@Emsu
Copy link
Contributor Author

Emsu commented Sep 27, 2018

@ekampf I've fixed the tests

It looks like tests are breaking due to upstream change of messages being unicode as well as slight error messaging changes. Perhaps we should freeze the graphene version and graphql-python version?

One other suggestion would be cutting a new release? I'm still working on graphene < 2.0 due to the releases in PyPi being the pre-graphene-2.0 graphene-gae release. Of course, if you feel the 2.0 release isn't ready yet, then I think it's fine not to release it, but then maybe master should be a continuation of graphene-gae 0.1.7 and we can keep the 2.0 branch separate.

@ekampf ekampf merged commit a223d10 into graphql-python:master Sep 27, 2018
@ekampf
Copy link
Collaborator

ekampf commented Sep 27, 2018

@Emsu I have migrated away from GAE a while back so I kinda have a hard time keeping this lib up to date.
Would you like to become a committer here so you can help with releasing new versions?

@Emsu
Copy link
Contributor Author

Emsu commented Sep 27, 2018

Sure @ekampf I'm happy to help out. I'm not working full-time with GAE so might not be the ideal candidate but I can at least do maintenance tasks and some small library updates.

@ekampf
Copy link
Collaborator

ekampf commented Sep 27, 2018

@syrusakbary can you please give @Emsu access?

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

Successfully merging this pull request may close these issues.

3 participants