-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[search source] Simplify / refactor flatten method - Part 2 #183206
[search source] Simplify / refactor flatten method - Part 2 #183206
Conversation
/ci |
5 similar comments
/ci |
/ci |
/ci |
/ci |
/ci |
// only include unique values | ||
if (sourceFieldsProvided) { | ||
if (!isEqual(remainingFields, fieldsFromSource)) { | ||
setWith(body, '_source.includes', remainingFields, (nsValue) => { |
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 is driving me crazy - this is object mutation code that I'd like to rewrite without object mutation but I'm having a very hard time figuring out how to do this.
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.
I wonder if this is all extraneous and could just be replaced with
if (sourceFieldsProvided && !isEqual(remainingFields, fieldsFromSource)) {
body._source.includes = remainingFields;
}
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
/ci |
1 similar comment
/ci |
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.
I think the readability improvements here are great! I wonder if we can do a little more to help the types from getting out of control - I know they were already bad to begin with, but I wonder if we can do something other than adding : any
so many times. I want to give this a more thorough look tomorrow and hopefully have some helpful feedback as to how we can accomplish that.
// only include unique values | ||
if (sourceFieldsProvided) { | ||
if (!isEqual(remainingFields, fieldsFromSource)) { | ||
setWith(body, '_source.includes', remainingFields, (nsValue) => { |
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.
I wonder if this is all extraneous and could just be replaced with
if (sourceFieldsProvided && !isEqual(remainingFields, fieldsFromSource)) {
body._source.includes = remainingFields;
}
@lukasolson I agree it would be great to improve the types. My earlier attempts didn't go well but it might be easier at this point
That doesn't work, at least not when I run the corresponding search_source.test.ts jest tests. |
/ci |
This seems to work: // only include unique values
if (sourceFieldsProvided && !isEqual(remainingFields, fieldsFromSource)) {
body._source = { includes: remainingFields };
} |
I took a stab at improving the types here: mattkime#21 |
/ci |
2276689
to
4065282
Compare
/ci |
2 similar comments
/ci |
/ci |
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.
cloud security changes ltgm
Co-authored-by: Matthias Wilhelm <ankertal@gmail.com>
/ci |
815cc33
to
2395f2a
Compare
/ci |
/ci |
💚 Build Succeeded
Metrics [docs]Canvas Sharable Runtime
Page load bundle
History
To update your PR or re-run it, just comment with: cc @mattkime |
Summary
Follow up to #183107 (should probably wait for that to be merged before reviewing this)
The SearchSource flatten method does a lot of object mutation and has a lot of long conditional statements. I'm breaking code out into functions and reducing the amount of object mutation. Changes to code logic have been kept minimal.
Part of #182068