php-github-api
php-github-api copied to clipboard
Added params argument for Labels and Search
Since we cannot use setPerPage() in 3.0, we need to be able to specify the per_page parameter without using a pager.
Why can't you use the lazy fetch method from the paginator, and only look at the first n things?
I only need the first 100 results. I am not interested in creating a paginator when I dont need it.
I've fixed the CS issues.
This PR will make the Search and Labels API more consistent with other APIs.
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.
Using fetchAllLazy would mean:
- I dont get IDE support anymore
- I need to instantiate a ResultPager
- 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.
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.
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:
- I dont get IDE support anymore
- I need to instantiate a ResultPager
- I need to have logic to count the results
- This is a valid point which is solved by this PR (for this usecase atleast)
- This is not really an issue for me
- Would it make sense to add a
fetchFirstResults($count)(or something like that) on theResultPager?
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). :)
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.