Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feature/results model #1307
Feature/results model #1307
Changes from 27 commits
10f7eef
bd0c78e
585ce6d
3793008
7462fff
9a187a6
f53879a
165441a
f5f6d17
2de499c
24d2ae8
4107349
690e0a6
3292677
36d4b82
85ecf40
fc1d972
5c7363c
502a8a9
26a30b2
914cd7c
7209873
5c4d1ef
e7ab8fa
e4b0a62
d98950e
e958ebd
6069ca8
267441c
800efcc
a00d5d4
71602a0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This method could use a (short) docstring
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.
How often do we expect to change this value? Could it be variable for different flavours of I-Analyzer? If so, this may belong in environment.
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.
Hm, good one. I think the more conventional way of handling it is to put a control for this next to pagination, so let users decide. But if I was going to work on expanding the results page right now, I would prioritise #1011, #1012, #729, or #996 - a size control seems low-priority to me. It may bring more clutter than value.
As for adding it to environment settings: that is technically trivial, but then I don't think have a reason to change this between our existing environments. Perhaps we could think more long-term about what we envision for different I-analyzer instances.
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.
Stray thought on this: size=20 is a bit trivial, since that is the elasticsearch default. Perhaps if we did not specify this, it could also be configured in the elasticsearch settings? Which would also make it environment-configurable.
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.
Needs a docstring