Skip to content

Feature/pagination#2056

Closed
BruceWW wants to merge 15 commits intofastapi:masterfrom
BruceWW:feature/pagination
Closed

Feature/pagination#2056
BruceWW wants to merge 15 commits intofastapi:masterfrom
BruceWW:feature/pagination

Conversation

@BruceWW
Copy link

@BruceWW BruceWW commented Sep 15, 2020

this is the feature that supports pagination,
with the param with_page_split as True, the method will calculate the length of result, split the result by page_num and page_size params and finally return previous url and next_url
pyandtic.BaseModel, sqlalchemy object and normal type are supported

@github-actions
Copy link
Contributor

📝 Docs preview for commit 83f97b7 at: https://5f60859297653e8b289530e9--fastapi.netlify.app

@codecov
Copy link

codecov bot commented Sep 15, 2020

Codecov Report

Merging #2056 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            master     #2056    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          239       241     +2     
  Lines         7079      7249   +170     
==========================================
+ Hits          7079      7249   +170     
Impacted Files Coverage Δ
fastapi/applications.py 100.00% <ø> (ø)
fastapi/pagination.py 100.00% <100.00%> (ø)
fastapi/routing.py 100.00% <100.00%> (ø)
tests/test_operations_signatures.py 100.00% <100.00%> (ø)
tests/test_response_with_page_split.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e77ea63...73ff0e9. Read the comment docs.

@github-actions
Copy link
Contributor

📝 Docs preview for commit 988db14 at: https://5f60a1a2ffcb39009bbb4904--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit 9033c5c at: https://5f60cc6a68e74a30a5d3e045--fastapi.netlify.app

Copy link
Contributor

@ycd ycd left a comment

Choose a reason for hiding this comment

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

I suggest you to use Typing's Dict and List for type-hinting since whole codebase based on Typing.

PR looks good overall 🎉 🚀

@BruceWW
Copy link
Author

BruceWW commented Sep 16, 2020

@ycd pertty agree with your suggestion, and it's been updated

@github-actions
Copy link
Contributor

📝 Docs preview for commit 92737a1 at: https://5f61ad396d3cd4279f8e5c7d--fastapi.netlify.app

@Shark009
Copy link

Any idea when this will be merged and released?

@Kludex
Copy link
Member

Kludex commented Sep 29, 2020

@Shark009 There's no confirmation that it will be merged...

@jrversteegh
Copy link

Despite its usefulness, this pagination option is not part of OpenAPI (as far as I am aware) and there are also competing options like page[number] and page[size] like used in json::api, so I don't think this should be in fastapi. Maybe in fastapi_utils or something?

@BruceWW
Copy link
Author

BruceWW commented Oct 10, 2020

Rest api do not suggest use json body while the request method is GET, so i think page[number] and page[size] is not a competitor. And fastapi_utils could be discussed

@github-actions
Copy link
Contributor

📝 Docs preview for commit 73ff0e9 at: https://5f815194f2fb3ef5b5d431bd--fastapi.netlify.app

@tiangolo
Copy link
Member

tiangolo commented Nov 5, 2020

I see you did a lot of work here @BruceWW ! 🤓

Nevertheless, that would be a very tight/strict compromise in how FastAPI expects to be used. And I would prefer to not have it that way. That's the same reason why it is not tightly coupled with any specific ORM.

For example, you could imagine someone wanting to use an opaque "cursor" object instead of pages. Or someone querying a NoSQL DB like CouchDB with an index range based on dates, etc.

So I'm gonna pass on this. Nevertheless, if this is a feature that you, your team, and/or others are frequently needing, I encourage you to create an independent Python package that can be used with FastAPI.

And if you add the fastapi topic to the GitHub repo it will show up here: https://fastapi.tiangolo.com/external-links/#projects

Given that, I'm gonna pass on this. But thanks for your effort! 🍰 ☕

@tiangolo tiangolo closed this Nov 5, 2020
@jrversteegh
Copy link

@BruceWW page[number] and page[size] are not body parameters, but query parameters in json::api. See e.g. this discussion if you're interested: OAI/OpenAPI-Specification#1706

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.

6 participants

Comments