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

Add pagination to listing packages #636

Merged
merged 1 commit into from
Jun 3, 2016
Merged

Add pagination to listing packages #636

merged 1 commit into from
Jun 3, 2016

Conversation

reset
Copy link
Collaborator

@reset reset commented Jun 2, 2016

  • Store package indices as sorted sets
  • Add ability to specify content ranges by the Range header to list the next portion of a long list of objects

Signed-off-by: Jamie Winsor [email protected]

@thesentinels
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @smith, @metadave, @fnichol and @adamhjk to be potential reviewers

@reset
Copy link
Collaborator Author

reset commented Jun 3, 2016

screen shot 2016-06-02 at 5 16 02 pm

@smith you'll need to hook up to the API additions for pagination but lexical sorting is done!

@reset reset force-pushed the pagination branch 3 times, most recently from 39bb51a to 919d38b Compare June 3, 2016 20:48
@@ -10,6 +10,7 @@ name = "bldr-api"
doc = false

[dependencies]
bodyparser = "*"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you even need bodyparser as a dep now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope! Just removed it

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it needs to be removed from this line also :) as well as in the Cargo.lock.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, we need it in the builder-api for some of my work coming up this week anyway. I'll leave it here for now

@smith
Copy link
Contributor

smith commented Jun 3, 2016

It works!

image

I think you can still fully remove the bodyparser dep though.

@reset
Copy link
Collaborator Author

reset commented Jun 3, 2016

@thesentinels r+

@smith thanks for the review and testing this out for me! Great direction on how to create pagination in 2016 😄

@thesentinels
Copy link
Contributor

📌 Commit 55a38fa has been approved by reset

@thesentinels
Copy link
Contributor

⌛ Testing commit 55a38fa with merge c64c10c...

thesentinels pushed a commit that referenced this pull request Jun 3, 2016
Signed-off-by: Jamie Winsor <[email protected]>

Pull request: #636
Approved by: reset
@smith
Copy link
Contributor

smith commented Jun 3, 2016

In order for this to work in a browser, we need to add Range to the Access-Control-Allow-Headers headers.

@thesentinels
Copy link
Contributor

☀️ Test successful - travis

@thesentinels thesentinels merged commit 55a38fa into master Jun 3, 2016
jtimberman pushed a commit that referenced this pull request Jun 12, 2016
Signed-off-by: Jamie Winsor <[email protected]>

Pull request: #636
Approved by: reset
@reset reset deleted the pagination branch June 14, 2016 08:16
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