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

Update panache documentation: zero-based page index fix #40703

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TyaraSchetgens
Copy link

To fetch page 7, Page with index 6 needs to be provided as index is zero-based
image

@quarkus-bot

This comment has been minimized.

Copy link

github-actions bot commented May 17, 2024

🎊 PR Preview 26e4b8a has been successfully built and deployed to https://quarkus-pr-main-40703-preview.surge.sh/version/main/guides/

  • Images of blog posts older than 3 months are not available.
  • Newsletters older than 3 months are not available.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

LGTM from what I see of the code but I must admit, it's a bit weird to use 0-based pages.

As a user, you will always talk about page 1, not page 0.

@gsmet
Copy link
Member

gsmet commented May 17, 2024

@FroMage I let you have a look at this one.

@TyaraSchetgens
Copy link
Author

LGTM from what I see of the code but I must admit, it's a bit weird to use 0-based pages.

As a user, you will always talk about page 1, not page 0.

Agreed. In our code we are doing page - 1 everywhere. It is a bit messy.

Would it be feasable to make the page index start at 1 instead of 0?

@gsmet
Copy link
Member

gsmet commented May 17, 2024

That would probably break all existing applications so that's probably something we won't be able to do unfortunately :/

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

To fetch page 7, Page with index 6 needs to be provided as index is zero-based
@quarkus-bot
Copy link

quarkus-bot bot commented May 28, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 7277d3a.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

I don't know about that. The javadocs of Page clearly mention it's zero-based. So "page 0" is the first page, and "page 7" is the eighth page. The comment you're refering to does not say "7th page" but "page 7", so it looks correct to me.

It's the same in most of the Java language. A List's first element will be at index 0.

Of course, both 0-based and 1-based indexing are valid, and depend on where they're being used. Spoken languages tend to use 1-based indexing, while math and C-inspired programming languages use 0-based indexing.

Using 0-based indexing in Java is the most intuitive and regular.

Perhaps this comment in the docs would have been clearer?

// get page 7 (pages are 0-based, so this is the 8th page in English)
List<Person> page7 = livingPersons.page(Page.of(7, 25)).list();

Would that help?

Quarkus Documentation automation moved this from To do to Review pending Jun 5, 2024
@gsmet
Copy link
Member

gsmet commented Jun 7, 2024

I don't know about that. The javadocs of Page clearly mention it's zero-based. So "page 0" is the first page, and "page 7" is the eighth page. The comment you're refering to does not say "7th page" but "page 7", so it looks correct to me.

I think you're considering that from a programming POV whereas pages are more often than not used in the UI.

So the initial gripe was really about I have my pagination with page 1-7 (because that's what you have on the user side) and I have to translate it to 0-based pages before pushing it to Panache. It's not a big deal but pagination is a lot about UI and how you present the data.
Now I don't think either perpective is right or wrong, it's just not very intuitive when working on the UI side.

That being said, this ship has sailed so we just need to make sure the doc is clear.

@FroMage
Copy link
Member

FroMage commented Jun 7, 2024

From a UI perspective you're right, but that's not the job of backends or APIs. The same is true when iterating indexes using Qute, everyone is used to adding +1 for humans.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants