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

Use "Link" instead of "Links" for header extra #142

Merged
merged 1 commit into from
Mar 11, 2019

Conversation

cseelus
Copy link
Contributor

@cseelus cseelus commented Mar 10, 2019

Just looked at the README for the headers feature and it really looked simple enough, so I couldn’t resist testing it.

Purging the api-pagination gem and replacing the API stuff was really straight forward, simple and quick.

At first glance with Postman everything looked ok, so I started up an App in iOS Simulator that consumes the API and paging didn’t work.
So looking closer at the links Pagy injects, I saw that you use

    Links →<…

instead of

    Link →<…

I think it would be hard to justify changing Apps that consume the API(s) the headers extra could be used for and RFC 8288 (page 14) also uses Link like api-pagination, so you might reconsider this.

@ddnexus
Copy link
Owner

ddnexus commented Mar 11, 2019

ooops... my bad. For some reason I had a sort of mind-typo that made me read and write Links in place of Link everywhere!

BTW I am not so sure about the naming of the other headers in #141. Is there any standard or should them be considered custom headers? Some recent gem use 'X-Total-Page' and 'X-Total-Count' and for custom headers the 'X-' prefix looks more correct to me.

I think I am going to add a :headers_map var or something to give the user the possibility to custom-name the headers how they want, but the default naming still puzzles me. Any suggestion?

@ddnexus ddnexus merged commit bcab431 into ddnexus:feature/headers Mar 11, 2019
@cseelus
Copy link
Contributor Author

cseelus commented Mar 11, 2019

Checked the new head of feature/headers and paging now works as expected.

There is nothing in the RFC regarding this. From what I've seen api-pagination gem uses Total while api-pager uses X-Total-Count.

The latter (prefixing X-) seems to make more sense, but I saw this was deprecated a while ago.

Edit: Making this configurable seems like a good idea, so people switching to Pagy for their APIs don't have to rewrite their consuming apps, if those headers are called differently in the API pagination gems they have used.

@ddnexus
Copy link
Owner

ddnexus commented Mar 11, 2019

I was not aware of the deprecation link. Thank you!

ddnexus added a commit that referenced this pull request Mar 12, 2019
* added headers extra
* Use "Link" instead of "Links" for header extra (#142)
* added customizable headers; normalized hash and headers; code improvements; updated doc and tests
@ddnexus ddnexus added the merged label Mar 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants