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

infinum/feature/combine-pagination-refactor #64

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Antonijo2307
Copy link
Contributor

…ct...

Description

Please include a summary of the feature you have added.

Checklist:

  • Have checked there is no same component already in catalog.
  • Have created a sufficient example or wrote tests for it.
  • Code is structured, well written using standard Swift coding style.
  • All public methods and properties have meaningful and concise documentation.
  • Used public modifier for all method and properties that could be changed from user of your feature, and private for internal properties.
  • Removed any reference to the project that piece of code was created in.
  • Reduced the dependencies to minimum.

@Antonijo2307 Antonijo2307 changed the title - refactored pagination so it can work with any Page || Pageable object… infinum/feature/combine-pagination-refactor Jan 11, 2023
Copy link
Contributor

@ZvonimirMedak ZvonimirMedak left a comment

Choose a reason for hiding this comment

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

I'd like @kvaljeva to take a look at this once he gets a chance before we do anything else 😄

Sources/Combine/Paging/PageablePresenter.swift Outdated Show resolved Hide resolved
Sources/Combine/Paging/PageablePresenter.swift Outdated Show resolved Hide resolved
Comment on lines 11 to 23
/// Defines template for page
protocol Page where Item: Pageable {

/// Constraint results to types that conform to Pageable
associatedtype Item

var count: Int { get }
var next: URL? { get }
var previous: URL? { get }
var pages: Int { get }
var page: Int { get }
var results: [Item] { get }
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why you went for an implementation where you would constrain it with where instead of just making it generic? Then you wouldn't need the associatedType if I'm not mistaken 😄

Suggested change
/// Defines template for page
protocol Page where Item: Pageable {
/// Constraint results to types that conform to Pageable
associatedtype Item
var count: Int { get }
var next: URL? { get }
var previous: URL? { get }
var pages: Int { get }
var page: Int { get }
var results: [Item] { get }
}
/// Defines template for page
protocol Page<Item: Pageable> {
var count: Int { get }
var next: URL? { get }
var previous: URL? { get }
var pages: Int { get }
var page: Int { get }
var results: [Item] { get }
}

Copy link
Contributor Author

@Antonijo2307 Antonijo2307 Jan 22, 2023

Choose a reason for hiding this comment

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

I have tried it and it seams that protocols can't have generics in this sense, it wont compile, it has to be associatedType. Maybe I didn't understand your suggestion

Comment on lines 48 to 53
func page(
loadNextPage: AnyPublisher<Void, Never>,
reload: AnyPublisher<Void, Never>,
nextPage: @escaping PageableResultClosure,
hasNextPage: @escaping HasNextPageClosure
) -> AnyPublisher<[Pageable], PagingError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this should be something that is contained inside the interactor, what do you think @kvaljeva? Maybe we could do something more along the lines of the NetwokPaginationService we did?

@nikolamajcen
Copy link
Member

Any status/update on this one? Its quite old, but let's decide if this will be improved and merged or closed.

@Antonijo2307
Copy link
Contributor Author

Antonijo2307 commented Jan 21, 2025

@nikolamajcen I have updated the code, just few answers from @ZvonimirMedak and @kvaljeva are needed to continue/finish.
And also @ZvonimirMedak maybe you could show me an example of NetwokPaginationService implementation so I may better understand your concerns and maybe help a bit in that direction

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