-
Notifications
You must be signed in to change notification settings - Fork 583
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
Add support for manually setting a limit on results #408
Conversation
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.
Thank you for this PR! I especially appreciate that you've added a configuration option for this setting.
I left a few minor comments. I also want to get some feedback from the rest of the team about whether or not we should allow for a limit, but still warn once that limit has been reached. If you can handle the few minor comments in the mean time, this PR should be good to merge.
api/api.go
Outdated
@@ -128,11 +129,25 @@ func parseAsUintValue(sv string, min, max, def uint) uint { | |||
return max | |||
} | |||
if min != 0 && uint(iv) < min { | |||
return max | |||
return min |
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.
Nice catch 🔥
api/api.go
Outdated
@@ -18,6 +18,7 @@ import ( | |||
const ( | |||
defaultLinesOfContext uint = 2 | |||
maxLinesOfContext uint = 20 | |||
maxLimit int = 100000 |
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.
Out of curiosity, is this number informed by anything or is it just reasonably large?
index/index_test.go
Outdated
@@ -59,6 +61,41 @@ func TestSearch(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestSearchWithLimits(t *testing.T) { |
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.
Thank you a million times over for adding tests for this.
api/api.go
Outdated
} | ||
return uint(iv) | ||
} | ||
|
||
func parseAsIntValue(sv string, min, max, def int) int { |
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.
Could I trouble you to write a few tests on the bounds checking logic for this and for parseAsUintValue
?
than a hard limit at 5000, for each index.
it remains at the former default of 5000 results per index.
exceeded - now it simply returns the results up to the limit.
Resolves #217