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

added 2 more fields to help with pagination as well as normalized key… #4

Merged
merged 1 commit into from
Sep 20, 2016

Conversation

jbhatab
Copy link
Contributor

@jbhatab jbhatab commented Sep 15, 2016

It seems the main mix spits out 4 keys and I felt we should just be replicating this to keep things consistent.

Here's what scrivener gives.
page_number: 1
page_size: 25
total_entries: 4
total_pages: 1

Now scrivener headers will give the following headers:

page-number: 1
page-size: 25
total-entries: 4
total-pages: 1

Copy link
Member

@doomspork doomspork left a comment

Choose a reason for hiding this comment

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

Not 100% on the key changes.

@@ -31,8 +31,10 @@ defmodule Scrivener.Headers do
query: conn.query_string}
conn
|> put_resp_header("link", build_link_header(uri, page))
|> put_resp_header("total", Integer.to_string(page.total_entries))
|> put_resp_header("per-page", Integer.to_string(page.page_size))
|> put_resp_header("total-pages", Integer.to_string(page.total_pages))
Copy link
Member

@doomspork doomspork Sep 16, 2016

Choose a reason for hiding this comment

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

What is the point of changing the header keys? It introduces a breaking change with previous versions and the gain is unclear.

@jbhatab
Copy link
Contributor Author

jbhatab commented Sep 16, 2016

@doomspork Yeah that's fair. The point was to just keep it consistent with the main scrivener mix, but I'll change it back to not break anything.

@doomspork
Copy link
Member

Thank you @jbhatab, I'll cut a new version shortly.

@doomspork doomspork merged commit 4997744 into beam-community:master Sep 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants