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

Added params argument for Labels and Search #1008

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

Conversation

Nyholm
Copy link
Collaborator

@Nyholm Nyholm commented May 13, 2021

Since we cannot use setPerPage() in 3.0, we need to be able to specify the per_page parameter without using a pager.

@GrahamCampbell
Copy link
Contributor

Why can't you use the lazy fetch method from the paginator, and only look at the first n things?

@Nyholm
Copy link
Collaborator Author

Nyholm commented May 14, 2021

I only need the first 100 results. I am not interested in creating a paginator when I dont need it.

@Nyholm
Copy link
Collaborator Author

Nyholm commented May 16, 2021

I've fixed the CS issues.

This PR will make the Search and Labels API more consistent with other APIs.

@GrahamCampbell
Copy link
Contributor

I'd recommend using fetchAllLazy and only grabbing the first 100 elements. Adding arbitrary params just leads to people not understanding the paginator exists and writing their own pagination. If it's not available to them, they discover the paginator.

@Nyholm
Copy link
Collaborator Author

Nyholm commented May 17, 2021

Using fetchAllLazy would mean:

  1. I dont get IDE support anymore
  2. I need to instantiate a ResultPager
  3. I need to have logic to count the results

Adding arbitrary params just leads to people not understanding the paginator exists and writing their own pagination.

It would also lead to that user can send any valid parameter to the API server... I think that is a good feature for the API client.

@dereuromark
Copy link
Contributor

I also do not see any harm in adding this PR

You can still document the alternative of a fetchAllLazy and how it works, its pros and cons etc, separately.

@acrobat
Copy link
Collaborator

acrobat commented May 24, 2021

While I think @GrahamCampbell is correct that we should point users to the ResultPager class, I also think it might make sense to add a $params array parameter to methods that point to api enpoints that have query, post variables etc. So let's merge this and "fix" other methods when the usecases come up.

On the points @Nyholm raised

Using fetchAllLazy would mean:

  1. I dont get IDE support anymore
  2. I need to instantiate a ResultPager
  3. I need to have logic to count the results
  1. This is a valid point which is solved by this PR (for this usecase atleast)
  2. This is not really an issue for me
  3. Would it make sense to add a fetchFirstResults($count) (or something like that) on the ResultPager?

@GrahamCampbell
Copy link
Contributor

Would it make sense to add a fetchFirstResults($count) (or something like that) on the ResultPager?

I think that would be a nice idea. Maybe just call it fetchFirst($n). :)

@Steveb-p
Copy link

Steveb-p commented Mar 7, 2024

I've stumbled upon this myself when trying to access subsequent pages of some GitHub API's within specific user web requests. I would expect to be able to query for a specific page (where I am entirely not interested in first couple pages). Otherwise I am required to perform http requests and iterate over results that I do not need.

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