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: make page[] param wrap consistent #348

Merged
merged 4 commits into from
Jun 28, 2019

Conversation

dugsmith
Copy link
Contributor

This is an opinionated fix to issue #347. It makes json_api_client handle pagination query params consistently by wrapping page[] around them in all places they are used. Of course, this can be overridden in custom paginators.

@dugsmith
Copy link
Contributor Author

@gaorlov: do you know when you might have a chance to consider this?

Copy link
Collaborator

@gaorlov gaorlov left a comment

Choose a reason for hiding this comment

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

Thank you so much for your contribution!
This looks to be a breaking change. Is there a way we can keep the existing interface as the default and emit a warning prompting to move away from it? Could we move the new interface to a different paginator?

I agree that what you're proposing is much cleaner and nicer to use, but the reason i'm pushing back is that this will cause unexpected and subtle pagination failures for upgrading users. Having battled paginators before, I have experienced just how hard those may be to track down.

How would you feel about moving the new paginator into something like HashedPaginator and adding a strongly worded statement to the README to use it instead of the current paginator. And then once we are ready to push the major version to v2 we can deprecate the existing paginator and make this one the default.

Again, thank you so much for taking the time! The code looks solid and I'm excited to get this merged in.

.tool-versions Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
@dugsmith
Copy link
Contributor Author

@gaorlov Thank you for your feedback! You're right, it is a breaking change. I will look into making this an alternate behavior while leaving the current behavior the default. That will make migration easier, for sure.

@dugsmith dugsmith force-pushed the feature/fix_pagination_issues branch from d069fd9 to a89d511 Compare June 27, 2019 18:36
@dugsmith dugsmith force-pushed the feature/fix_pagination_issues branch from a89d511 to cc4b697 Compare June 27, 2019 18:37
@dugsmith
Copy link
Contributor Author

@gaorlov I've updated this to use a NestedParamPaginator as the one that has consistently nested behavior across all use-cases. The original behavior is the default. How does this look?

Copy link
Collaborator

@gaorlov gaorlov left a comment

Choose a reason for hiding this comment

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

This is excellent! Thanks so much for your work!
Final minor ask: can you add a note to the readme and the changelog?
Thanks again so much. I'm excited to get this merged.

@dugsmith
Copy link
Contributor Author

Thanks, @gaorlov! I've updated README and CHANGELOG. I assumed you'd want to increment the version when you merge, but let me know if you need me to add that commit too.

@dugsmith
Copy link
Contributor Author

@gaorlov Did you want me to bump the gem version before you merge this to master?

@gaorlov
Copy link
Collaborator

gaorlov commented Jun 28, 2019

@dugsmith I'll bump the version in master. I'll merge it on and ship the green in the next hour or so. Thank you for your work!

@dugsmith
Copy link
Contributor Author

@gaorlov: Excellent! Thank you for shepherding this great project!

@gaorlov gaorlov merged commit cc36a9d into JsonApiClient:master Jun 28, 2019
@gaorlov
Copy link
Collaborator

gaorlov commented Jun 28, 2019

@dugsmith 1.13.0 is now live. Thank you for your work and for accommodating my requests. I really appreciate it.

@dugsmith
Copy link
Contributor Author

@dugsmith 1.13.0 is now live. Thank you for your work and for accommodating my requests. I really appreciate it.

@gaorlov Thank you for turning this around so quickly! It helps us a lot! :-)

@dugsmith dugsmith deleted the feature/fix_pagination_issues branch July 19, 2019 20:26
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.

2 participants