Skip to content

Added params argument for Labels and Search#1008

Open
Nyholm wants to merge 2 commits into
KnpLabs:masterfrom
Nyholm:params
Open

Added params argument for Labels and Search#1008
Nyholm wants to merge 2 commits into
KnpLabs:masterfrom
Nyholm:params

Conversation

@Nyholm

@Nyholm Nyholm commented May 13, 2021

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Contributor

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

@Nyholm

Nyholm commented May 14, 2021

Copy link
Copy Markdown
Collaborator Author

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

@Nyholm

Nyholm commented May 16, 2021

Copy link
Copy Markdown
Collaborator Author

I've fixed the CS issues.

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

@GrahamCampbell

Copy link
Copy Markdown
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

Nyholm commented May 17, 2021

Copy link
Copy Markdown
Collaborator Author

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
Copy Markdown
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

acrobat commented May 24, 2021

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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

Steveb-p commented Mar 7, 2024

Copy link
Copy Markdown

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