-
Notifications
You must be signed in to change notification settings - Fork 4
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
Set the default limit when retrieving saved searches to 10 #63
base: main
Are you sure you want to change the base?
Conversation
…anetlabs/client into ericrdunham/limit-saved-searches-list
@@ -54,7 +54,7 @@ function search(options) { | |||
var config = { | |||
url: urls.searches(), | |||
query: query, | |||
limit: options.limit, | |||
limit: 'limit' in options ? options.limit : 10, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? If the desired behavior is to have a default of 10, then the API should be the one enforcing that. In this case if options.limit
is empty or null the query param is not added to the request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bcvazquez this is needed to prevent api/pager
from infinitely trying to consume pages from Data API.
This configuration value gets passed through to the pager which defaults to Infinity
and consumes all pages, so it wouldn't matter what the default for Data API is (it's 250, for reference). The way it looks right now, no options ever get added, so the pager tries to consume all results, in pages/chunks of 250 at a time from the Data API.
The change here limits the scope of the defaulting strictly to retrieving saved searches (as far as I'm aware), because while I personally think that trying to retrieve infinity anything isn't a good idea, I'm not familiar enough with use cases in the CLI and in Explorer to know if changing the value in api/pager
would break other things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦♀ ... sorry Eric, I guess I didn't fully trace this nor understood the pager. Thanks for explaining it!
And yes, api/pager gets used in other places, and this seems like the safest bet.
This mirrors planetlabs/planet-client-python#178. This pull request would allow for sane display limits for saved searches in a terminal, Explorer, or other application.