php-github-api icon indicating copy to clipboard operation
php-github-api copied to clipboard

Added params argument for Labels and Search

Open Nyholm opened this issue 4 years ago • 9 comments

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

Nyholm avatar May 13 '21 09:05 Nyholm

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

GrahamCampbell avatar May 13 '21 23:05 GrahamCampbell

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

Nyholm avatar May 14 '21 06:05 Nyholm

I've fixed the CS issues.

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

Nyholm avatar May 16 '21 14:05 Nyholm

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.

GrahamCampbell avatar May 16 '21 15:05 GrahamCampbell

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.

Nyholm avatar May 17 '21 07:05 Nyholm

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.

dereuromark avatar May 18 '21 15:05 dereuromark

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?

acrobat avatar May 24 '21 10:05 acrobat

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). :)

GrahamCampbell avatar May 24 '21 13:05 GrahamCampbell

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.

Steveb-p avatar Mar 07 '24 10:03 Steveb-p