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

Infer cursor fields from simple order_by statements. (WIP) #41

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

Conversation

Nickforall
Copy link
Contributor

@Nickforall Nickforall commented Nov 12, 2018

This is a version that parses the order fields and their corresponding directions from the Ecto Query struct.

I will rebase this branch and continue as soon as the features of #34 have landed, maybe we can release this together with the new sorting feature?

Todo's

@bernardd
Copy link
Contributor

Hi @stevedomin - Is there anything I can do to help get this merged in? I'm using this functionality for a feature over in Absinthe and I'd really like to have an official hex package to base that on rather than just pointing it to a fork/branch somewhere.

@Nickforall
Copy link
Contributor Author

I'll take some time to make this ready for review/merge this week.

@fuelen
Copy link

fuelen commented Apr 1, 2019

Any progress?

@bernardd
Copy link
Contributor

Hey @stevedomin - this PR is working well for our use case. Is there anything I can do to help get it merged? Sorry to keep hassling you :)

@Nickforall
Copy link
Contributor Author

It literally doesn't do anything yet. There's a module that resolves cursor fields from order-bys, but its method are not used anywhere in the pagination logic yet, I have to get familiar with the code base again, and I'm travelling right now.

Sorry I couldn't make it work earlier, but a lot of things came in between

@bernardd
Copy link
Contributor

bernardd commented Apr 15, 2019

Really? Maybe I'm just using the #34 changes then and...somehow it's doing everything I need anyway. Apologies - apparently I also need to get familiar with the codebase again :)

Edit: Yep, I'm an idiot. Turns out I can more or less make do with #34 (which is already merged) with just a couple of tiny changes to our code. Though it would still be great to get this one going :)

@Nickforall
Copy link
Contributor Author

It might be that it's sorting by id (that's the default if nothing is given iirc), and that it happens to work for your test case :)

@bernardd
Copy link
Contributor

It might be that it's sorting by id (that's the default if nothing is given iirc), and that it happens to work for your test case :)

It was actually just that I forgot exactly what this one was meant to do and thought "oh, he must do stuff slightly differently" and managed to get away with making a tiny change and it was enough. So yeah, entirely me not paying close enough attention/remembering what this was about in the first place :)

Base automatically changed from master to main January 26, 2021 17:52
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.

3 participants