Skip to content
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

Typescript es-query functions #57473

Merged
merged 22 commits into from
Feb 27, 2020

Conversation

lizozom
Copy link
Contributor

@lizozom lizozom commented Feb 12, 2020

Summary

Part of #56881

Basic Typescript for es-query functions

Checklist

Delete any items that are not applicable to this PR.

For maintainers

Sorry, something went wrong.

@lizozom lizozom requested a review from a team as a code owner February 12, 2020 17:19
@lizozom lizozom self-assigned this Feb 12, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@lizozom
Copy link
Contributor Author

lizozom commented Feb 12, 2020

@elasticmachine merge upstream

Liza K added 2 commits February 12, 2020 19:37
…izozom/kibana into newplatform/data/ts-es-query-functions
@lizozom lizozom requested review from lukasolson and Bargs and removed request for lukeelmers February 12, 2020 17:40
Liza K added 2 commits February 12, 2020 19:56

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@lizozom
Copy link
Contributor Author

lizozom commented Feb 15, 2020

@elasticmachine merge upstream

Copy link
Contributor

@Bargs Bargs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Types look reasonable to me from just reading the code. I wasn't able to bootstrap though due to a type error in function.ts. Otherwise LGTM. Definitely have @lukasolson take a look at this too before merging, I just transferred all my most recent KQL knowledge to him so he'll be the best reviewer for these sorts of changes going forward.

Liza K added 5 commits February 25, 2020 19:50

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…izozom/kibana into newplatform/data/ts-es-query-functions
Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some notes below, just a few minor changes requested and nits.

Just wanted to document a few improvements we could continue to make after this is in:

  • Create types for each of the different "function" node types (e.g. AndNode, OrNode, etc.)
  • Create type for config (I think it contains things like allowLeadingWildcards and potentially other UI settings)
  • Create type for context (which I think only contains things about whether it's nested or not, but this would take some research)
  • Add types for the params given to geoBoundingBox/geoPolygon/nested
  • Parameterize the literal.buildNode (so you can do something like literal.buildNode<string>)
  • Potentially add types for the results of toElasticsearchQuery (although I think these would probably belong in our elasticsearch dependency)
  • Add types for the fieldName arg (that could be a node/string I believe) that is passed in several places
  • Get rid of the as LiteralTypeBuildNode by adding is functions that are exported from types
  • Audit where we've added @ts-ignore statements and build out the parameterized types to get rid of them

src/plugins/data/common/es_query/kuery/functions/and.ts Outdated Show resolved Hide resolved
src/plugins/data/common/es_query/kuery/functions/and.ts Outdated Show resolved Hide resolved
src/plugins/data/common/es_query/kuery/functions/not.ts Outdated Show resolved Hide resolved
src/plugins/data/common/es_query/kuery/functions/or.ts Outdated Show resolved Hide resolved
src/plugins/data/common/es_query/kuery/functions/or.ts Outdated Show resolved Hide resolved
src/plugins/data/common/es_query/kuery/functions/range.ts Outdated Show resolved Hide resolved
src/plugins/data/common/es_query/kuery/functions/range.ts Outdated Show resolved Hide resolved
@lizozom lizozom requested a review from lukasolson February 26, 2020 20:45
Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@lizozom lizozom merged commit 6ecba58 into elastic:master Feb 27, 2020
lizozom pushed a commit to lizozom/kibana that referenced this pull request Feb 27, 2020
* TS Functions

* Rename index.ts

* tsing

* JSON Value

* ts ignore

* TS adjustments

* context?.nested?.path

* Code review

* Import LiteralTypeBuildNode

* Fix jest tests

* revert test

* TS fix IFieldType

* Remvoe unnecessary casting

* range types

Co-authored-by: Elastic Machine <[email protected]>
lizozom pushed a commit that referenced this pull request Feb 27, 2020
* TS Functions

* Rename index.ts

* tsing

* JSON Value

* ts ignore

* TS adjustments

* context?.nested?.path

* Code review

* Import LiteralTypeBuildNode

* Fix jest tests

* revert test

* TS fix IFieldType

* Remvoe unnecessary casting

* range types

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants