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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix jsonapi prev and next keys for unavailable links #665

Merged
merged 2 commits into from
Mar 16, 2024

Conversation

kobusjoubert
Copy link
Contributor

Hi @ddnexus, thank you so much for this awesome Gem 馃拵

I came across an issue when using the Jsonapi extra.

The Problem

I'm getting a expected :page >= 1; got 0 exception when following a prev or next link with no content.

Screenshot 2024-03-15 at 07 58 01
Screenshot 2024-03-15 at 08 23 50

The Fix

According to the JSON:API spec, it specifies to omit the key or return null for unavailable links.

Screenshot 2024-03-15 at 08 33 12

If you're okay with this, I've applied a small change to return null for unavailable links in this pull request.

Screenshot 2024-03-15 at 10 51 26
Screenshot 2024-03-15 at 10 51 43
Screenshot 2024-03-15 at 10 52 01

Let me know if you want me to change anything @ddnexus.

@ddnexus
Copy link
Owner

ddnexus commented Mar 15, 2024

Hi @kobusjoubert . Thanks for the PR.

I am conflicted about using null values or omitting... I even researched it and... it looks everybody is conflicted about null or omit. It looks like the two options are equally... meh! 馃檭

So... the option that you choose looks visually better in the code, so that seems to be compelling enough for me. 馃榿

Please, could you fix the tests. Thank you!

@kobusjoubert
Copy link
Contributor Author

Hi @ddnexus, that's awesome! Thank you so much for looking at this pull request 馃コ

I just added some tests, runs good on my side.

@ddnexus ddnexus merged commit 74b4e50 into ddnexus:master Mar 16, 2024
5 checks passed
@ddnexus
Copy link
Owner

ddnexus commented Mar 16, 2024

Thank you!

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.

None yet

2 participants