Skip to content

New Connection API #347

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

New Connection API #347

wants to merge 6 commits into from

Conversation

syrusakbary
Copy link
Member

@syrusakbary syrusakbary commented Nov 3, 2016 •
edited
Loading

This PR improves the relay.Connection APIs in Graphene.
Is inspired on the work of @Globegitter in #288 and following #304.

Things that this PR should achieve:

  • Remove automatic .Connection creation in Nodes in pro of cleaner and more explicit code
  • Remove the graphql-relay dependency.
  • One connection type could have different resolving strategies (different cursors / In a fully list, in a iterable without an explicit length = django queryset, ...)

Related issues

#304 #180 #62

@coveralls
Copy link

coveralls commented Nov 3, 2016 •
edited
Loading

Coverage Status

Coverage decreased (-0.4%) to 95.487% when pulling 42b93b5 on features/connection into adfbffb on master.

Globegitter
Globegitter reviewed Nov 3, 2016
View reviewed changes
graphene/relay/connection.py Outdated
if isinstance(resolved, connection_type):
return resolved

on_resolve = partial(connection_type.connection_resolver, args=args, context=context, info=info)
if isinstance(resolved, Promise):
Copy link

@Globegitter Globegitter Nov 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only minor problem I have with this is that the instance check does not allow for duck-typing, which goes in line with graphql-python/graphql-core#91 and is a quite common principle in python.

graphene/relay/connection.py Outdated
@classmethod
def connection_resolver(cls, resolver, connection_type, root, args, context, info):
resolved = resolver(root, args, context, info)

on_resolve = partial(cls.resolve_connection, connection_type, args)
if isinstance(resolved, connection_type):
Copy link

@Globegitter Globegitter Nov 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Support for duck-typing would be nice here as well but I think it would probably be far less used and more a nit-pick from my side.

@Globegitter
Copy link

Liking the direction this is heading so far :)

@coveralls
Copy link

coveralls commented Nov 4, 2016 •
edited
Loading

Coverage Status

Coverage decreased (-0.4%) to 95.496% when pulling 3fa65aa on features/connection into adfbffb on master.

@coveralls
Copy link

coveralls commented Nov 4, 2016 •
edited
Loading

Coverage Status

Coverage decreased (-0.4%) to 95.496% when pulling e6733d5 on features/connection into adfbffb on master.

@Globegitter Globegitter mentioned this pull request Nov 10, 2016
Closed
@syrusakbary syrusakbary mentioned this pull request Nov 16, 2016
Closed
@tsunammis
Copy link

Hello @syrusakbary
Since your commit -> 760ccc8, Is it not possible to use async resolver method for a relay.ConnectionField, isn't it ?

I get this error -> AssertionError: Resolved value from the connection field have to be iterable or instance of SectionConnection. Received "<coroutine object get_sections at 0x7f0de4b14518>"

@ajhyndman
Copy link

Hey guys. I'm wondering what the status is on this PR. It seems like some of the documented Connection APIs might be out of date?

I'm getting errors that look like this when I try to declare a property with type ConnectionField(MyNode):

16, in is_node
    if not issubclass(objecttype, ObjectType):
TypeError: issubclass() arg 1 must be a class

@syrusakbary syrusakbary mentioned this pull request Jul 12, 2017
Merged
23 tasks
@syrusakbary
Copy link
Member Author

Closing in favor of #500

@ProjectCheshire ProjectCheshire deleted the features/connection branch June 2, 2019 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@Globegitter Globegitter Globegitter left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@syrusakbary @coveralls @Globegitter @tsunammis @ajhyndman