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 hasNext and hasPrevious properties #482

Closed
wants to merge 4 commits into from

Conversation

noahpistilli
Copy link

This PR adds the properties hasNext and hasPrevious which are similar to the flask-sqlalchemy properties has_next and has_prev.

These properties are similar to the flask-sqlalchemy properties `has_next` and `has_prev`.
@0xTim
Copy link
Member

0xTim commented Feb 17, 2022

Can you add some tests to this? Especially to check the boundaries/edge cases. Like count of 0, trying to pass in a page of -1 etc

Comment on lines +91 to +96
let remainingItems = total - (per * page)
if remainingItems <= 0 {
self.hasNext = false
} else {
self.hasNext = true
}

Choose a reason for hiding this comment

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

How do we feel about one-liners like below?

Suggested change
let remainingItems = total - (per * page)
if remainingItems <= 0 {
self.hasNext = false
} else {
self.hasNext = true
}
self.hasNext = (total - (per * page)) <= 0
self.hasPrevious = page > 1

Copy link
Member

Choose a reason for hiding this comment

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

Personally I'd choose the existing code, it's a bit easier to read at a glance. But I don't have a strong opinion on it

@noahpistilli
Copy link
Author

noahpistilli commented Feb 20, 2024

Can you add some tests to this? Especially to check the boundaries/edge cases. Like count of 0, trying to pass in a page of -1 etc

Should I add bounds to PageMetadata.page and PageMetadata.per then? Right now it will gladly accept a negative page number and a value of 0

@0xTim
Copy link
Member

0xTim commented Feb 20, 2024

Can you add some tests to this? Especially to check the boundaries/edge cases. Like count of 0, trying to pass in a page of -1 etc

Should I add bounds to PageMetadata.page and PageMetadata.per then? Right now it will gladly accept a negative page number and a value of 0

Yes we definitely need tests and count checking for this

@codecov-commenter
Copy link

codecov-commenter commented Feb 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 24.90%. Comparing base (614d3ec) to head (e796c6a).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #482      +/-   ##
==========================================
+ Coverage   24.78%   24.90%   +0.12%     
==========================================
  Files         149      149              
  Lines        8660     8670      +10     
==========================================
+ Hits         2146     2159      +13     
+ Misses       6514     6511       -3     
Files with missing lines Coverage Δ
...luentKit/Query/Builder/QueryBuilder+Paginate.swift 84.48% <100.00%> (+9.48%) ⬆️

@gwynne
Copy link
Member

gwynne commented Sep 22, 2024

Unfortunately, because PageMetadata is both public and Codable, this is a breaking change, and at this point I'm no longer accepting new features for Fluent 4. That being said, this functionality will be available in Fluent 5.

@gwynne gwynne closed this Sep 22, 2024
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.

5 participants