Fix pagination indicators when using list slices #14
Conversation
|
The remaining failing tests are unrelated and seem to be due to changes in the latest version of graphql-python. |
sciyoshi
force-pushed
the
pagination-indicators
branch
from
bede3a2 to
c45a687
Compare
|
I think we really need this to be merged, would it be possible to have this fixed? |
|
Ping @syrusakbary |
|
👋 is there something we can do to get this sucker merged? |
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
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
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
|
@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. |
|
@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 |
|
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. |
|
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
force-pushed
the
pagination-indicators
branch
from
c45a687 to
a2df9a8
Compare
|
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. |
|
@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 :)) |
|
I haven't done any work with django and graphene, so not sure my self. |
|
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. |
According to the spec,
hasNextPageandhasPreviousPageshould be set regardless of the pagination direction if they can be efficiently computed.Fixes #12