-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Data] Cleanup filter docs #107169
[Data] Cleanup filter docs #107169
Conversation
… data-cleanup/filters
Pinging @elastic/kibana-app-services (Team:AppServices) |
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.
core changes lgtm: fixing a comment typo in src/core/public/fatal_errors/fatal_errors_service.tsx
* @public | ||
*/ | ||
export const isExistsFilter = (filter: FieldFilter): filter is ExistsFilter => | ||
get(filter, 'exists'); |
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 function doesn't explicitly return a boolean, the return value of get
is any
.
Should this be:
get(filter, 'exists'); | |
get(filter, 'exists') != null; |
?
If it's valid for filter.exists
to be 0
, then this function seems it would return the wrong value. Perhaps this is not a big deal, and also the same issue existed in the previous code.
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.
Fixed it by replacing get
with has
which does exactly that
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.
Great job on this! It looks very thorough.
I left a comment about a pattern of code that uses get
to determine an object type, and it looks a bit loose to me. But I wouldn't block you from merging this PR based on my comment.
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
Unknown metric groupsAPI count missing comments
API count with any type
History
To update your PR or re-run it, just comment with: cc @lizozom |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
2 similar comments
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
* Move more utils to package and cleanup API * docs and imports * better imports * change comment * Better docs * typos * typo * fixes * casting * Code review * Update meta_filter.ts * fix Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # docs/development/plugins/data/public/kibana-plugin-plugins-data-public.castestokbnfieldtypename.md # docs/development/plugins/data/public/kibana-plugin-plugins-data-public.getkbntypenames.md # docs/development/plugins/data/public/kibana-plugin-plugins-data-public.md # docs/development/plugins/data/server/kibana-plugin-plugins-data-server.buildqueryfromfilters.md # docs/development/plugins/data/server/kibana-plugin-plugins-data-server.castestokbnfieldtypename.md # docs/development/plugins/data/server/kibana-plugin-plugins-data-server.esqueryconfig.md # docs/development/plugins/data/server/kibana-plugin-plugins-data-server.filter.md # docs/development/plugins/data/server/kibana-plugin-plugins-data-server.ifieldsubtype.md # docs/development/plugins/data/server/kibana-plugin-plugins-data-server.kuerynode.md # src/plugins/data/public/public.api.md
* Update SM doc for alert per object (#107420) Update stack monitoring doc to account for alert notification now being send for each node, index, or cluster based on the rule type, instead of always per cluster (PR# 102544) * [Data] Cleanup filter docs (#107169) * Move more utils to package and cleanup API * docs and imports * better imports * change comment * Better docs * typos * typo * fixes * casting * Code review * Update meta_filter.ts * fix Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # docs/development/plugins/data/public/kibana-plugin-plugins-data-public.castestokbnfieldtypename.md # docs/development/plugins/data/public/kibana-plugin-plugins-data-public.getkbntypenames.md # docs/development/plugins/data/public/kibana-plugin-plugins-data-public.md # docs/development/plugins/data/server/kibana-plugin-plugins-data-server.buildqueryfromfilters.md # docs/development/plugins/data/server/kibana-plugin-plugins-data-server.castestokbnfieldtypename.md # docs/development/plugins/data/server/kibana-plugin-plugins-data-server.esqueryconfig.md # docs/development/plugins/data/server/kibana-plugin-plugins-data-server.filter.md # docs/development/plugins/data/server/kibana-plugin-plugins-data-server.ifieldsubtype.md # docs/development/plugins/data/server/kibana-plugin-plugins-data-server.kuerynode.md # src/plugins/data/public/public.api.md * doc Co-authored-by: Ravi Kesarwani <64450378+ravikesarwani@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* Move more utils to package and cleanup API * docs and imports * better imports * change comment * Better docs * typos * typo * fixes * casting * Code review * Update meta_filter.ts * fix Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Summary
any
types wherever possibleChecklist
Delete any items that are not applicable to this PR.
Risk Matrix
Delete this section if it is not applicable to this PR.
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:
For maintainers