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

Fix pagination indicators when using list slices #14

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sciyoshi
Copy link

According to the spec, hasNextPage and hasPreviousPage should be set regardless of the pagination direction if they can be efficiently computed.

Fixes #12

@sciyoshi
Copy link
Author

The remaining failing tests are unrelated and seem to be due to changes in the latest version of graphql-python.

@sebastiandev
Copy link

I think we really need this to be merged, would it be possible to have this fixed?

@sciyoshi
Copy link
Author

Ping @syrusakbary

@airhorns
Copy link

👋 is there something we can do to get this sucker merged?

open-dynaMIX added a commit to open-dynaMIX/caluma that referenced this pull request Jun 3, 2019
This commit implements a fix for
graphql-python/graphql-relay-py#12

The project `graphql-relay-py` seems unmaintained, so there is
little hope that
[this PR](graphql-python/graphql-relay-py#14)
gets merged any time soon.

Closes projectcaluma#469
open-dynaMIX added a commit to open-dynaMIX/caluma that referenced this pull request Jun 3, 2019
This commit implements a fix for
graphql-python/graphql-relay-py#12

The project `graphql-relay-py` seems unmaintained, so there is
little hope that
[this PR](graphql-python/graphql-relay-py#14)
gets merged any time soon.

Closes projectcaluma#469
open-dynaMIX added a commit to open-dynaMIX/caluma that referenced this pull request Jun 3, 2019
This commit implements a fix for
graphql-python/graphql-relay-py#12

The project `graphql-relay-py` seems unmaintained, so there is
little hope that
[this PR](graphql-python/graphql-relay-py#14)
gets merged any time soon.

Closes projectcaluma#469
@sliverc
Copy link

sliverc commented Jul 8, 2019

@mvanlonden it seems that you merged the last PR quite recently. Could you have a look at this PR as well? It simply needs a rebase to fix CI. Thx.

@Cito
Copy link
Member

Cito commented Jul 8, 2019

@sciyoshi Just to clarify: Do you think this is a bug in the reference implementation graphql-relay-js or a bug in how graphql-relay-py implements it?

It looks like the former since you changed tests which have been taken over 1:1 from JS. I did not have time yet to look deeper into this issue but generally I don't think it's good if the Python version behaves differently from the JS one. Better to report and discuss such problems upstream before deviating from the behavior and functionality of the JS library. See also my comment in #16

@sciyoshi
Copy link
Author

sciyoshi commented Jul 9, 2019

Thank you @Cito for the reply, that's a great question and I wasn't aware that this was a direct port of the reference JS implementation. Reading the discussion graphql/graphql-relay-js#58, I would argue that it is an issue in graphql-relay-js that hasn't yet been fixed. The discussion there led to this PR which updates the spec with this new behavior, but it seems like no one has done the work to implement it. With regards to whether it makes sense for the behavior to be implemented in this library first I'll leave up to the maintainers, but it seems like this has been requested by others before.

@sliverc I would be happy to rebase this PR and fix the tests, but it would be good to know if this is something that there is interest in merging first.

@sciyoshi
Copy link
Author

sciyoshi commented Jul 9, 2019

As another data point, the Ruby port added bi-directional pagination support using a setting.

According to the spec, if hasNextPage and hasPreviousPage should be set
regardless of the pagination direction if they can be efficiently computed.
@sciyoshi
Copy link
Author

sciyoshi commented Aug 6, 2019

@Cito @sliverc I've updated this PR against the latest changes in master. Any updates on whether this is something that could be merged?

@Cito
Copy link
Member

Cito commented Aug 6, 2019

We can consider this after releasing 3.0 in the next version 3.1.

It would be best if this would changed upstream in the JS version, so I will try that first. If this will not be possible or takes too long, we should try to implement this in a backward compatible way, i.e. by adding a parameter or using different function name.

@tony
Copy link

tony commented Jan 1, 2020

@sciyoshi Can you do a rebase?

Can you make a forked repo for 2.x ?

@sebastiandev @Cito Is there a way you'd be willing to show how we could override the connection behavior without needing to fork the project? Preferably/possibly from graphene-django (though I realize its a far away repo :))

@sebastiandev
Copy link

I haven't done any work with django and graphene, so not sure my self.

@Cito
Copy link
Member

Cito commented Jan 19, 2022

I'm currently trying to bring relay-py in line with the current versions of core and relay-js, and then reconsider the pending PRs and issues. Unfortunately, currently we cannot deploy new versions to PyPI because travis-ci.org stopped working. We need @syrusakbary to help with this, I already contacted him.

Regarding this PR, see also my above comments: The goal of this lib is to replicate relay-js - it should not have a different behavior. So I would not want to merge this PR as-is. The two options to move this forward are: 1) to promote a change in relay-js or 2) make the behavior configurable or overridable while keeping the standard behavior in line with relay-js.

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.

has_previous_page and has_next_page always False while navigating
6 participants