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

v1.1.0 #43

Merged
merged 40 commits into from
Feb 17, 2024
Merged

v1.1.0 #43

merged 40 commits into from
Feb 17, 2024

Conversation

ewilan-riviere
Copy link
Collaborator

@ewilan-riviere ewilan-riviere commented Feb 16, 2024

@ewilan-riviere ewilan-riviere self-assigned this Feb 16, 2024
Copy link

codecov bot commented Feb 16, 2024

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (d6ef3bd) 99.43% compared to head (ae90bbd) 99.13%.

Files Patch % Lines
src/Engine/Paginate/OpdsPaginate.php 97.97% 2 Missing ⚠️
src/Engine/OpdsEngine.php 87.50% 1 Missing ⚠️
src/OpdsConfig.php 83.33% 1 Missing ⚠️
src/OpdsResponse.php 87.50% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main      #43      +/-   ##
============================================
- Coverage     99.43%   99.13%   -0.31%     
- Complexity      259      313      +54     
============================================
  Files            10       12       +2     
  Lines           890     1042     +152     
============================================
+ Hits            885     1033     +148     
- Misses            5        9       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ewilan-riviere
Copy link
Collaborator Author

ewilan-riviere commented Feb 16, 2024

@mikespub
To help for transparency, I offer to add a method pagination() or paginate() (tell me the one your like) to update pagination system.

If pagination() method have no param automatic pagination will be executed on it (previous pagination system) but with param, a OpdsPaging::class instance (you can see current OpdsPagingTest.php), manual pagination will be executed. The aim is to merge two systems of pagination.

And usePagination and useAutoPagination will be removed from OpdsConfig.

What do you think about this?
And can you tell me if current handle of manual pagination is like you want?

@mikespub
Copy link
Contributor

Hi @ewilan-riviere I'm fine with re-using paginate() if that's what you want, but I assume you'll somehow want to combine the functionality of the original paginate() with the new paging() I added then?

The reason I'm asking is because the paging() I used in the PR deals with "opaque" URLs as strings coming from the application, and then uses those directly in the feeds - which is why I used some random variations with f=.., n=.., p=.., l=... params for each in the Paging Test. It's not at all what the links actually look like in COPS, but it shouldn't matter :-)

While the original paginate() tries to count items, pages, previous & next numbers etc. as integers, and then truncates the feed(s) and generates URL links for them - something completely different...

@ewilan-riviere
Copy link
Collaborator Author

ewilan-riviere commented Feb 16, 2024

@mikespub Thanks for quick answer!

Yes, it's exactly what you done in your PR, I just update it with a new class to handle this with an object, like in your test OpdsPagingTest.

The aim is to combine two paginations yes.

// no pagination
$opds = Opds::make(getConfig())
        ->feeds(manyFeeds())
        ->get();

// with pagination (automatic)
$opds = Opds::make(getConfig())
        ->feeds(manyFeeds())
        ->pagination()
        ->get();

// with pagination (manual)
$opds = Opds::make(getConfig())
        ->feeds(manyFeeds())
        ->pagination(new OpdsPaging(
            currentPage: $page,
            totalItems: $total,
            firstUrl: 'http://localhost:8080/opds?f=1',
            lastUrl: 'http://localhost:8080/opds?l=42',
            previousUrl: 'http://localhost:8080/opds?p=1',
            nextUrl: 'http://localhost:8080/opds?n=3',
        ))
        ->get();

This is an example, for now pagination auto is into OpdsConfig and your new method paging() allow to use manual pagination. But it should be possible to combine two paginations.

EDIT
Tell me if I'm not clear

@ewilan-riviere
Copy link
Collaborator Author

@mikespub
It's done, can you tell me if it's ok for you?

@mikespub
Copy link
Contributor

Ah yes, now I see - thanks, that's good for me too :-)

@ewilan-riviere ewilan-riviere merged commit 231f199 into main Feb 17, 2024
10 of 12 checks passed
This pull request was closed.
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.

2 participants