Skip to content

Generate a static parser, move tests to vis_type_timelion #55299

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jan 21, 2020
Merged

Generate a static parser, move tests to vis_type_timelion #55299

merged 3 commits into from
Jan 21, 2020

Conversation

@sulemanof
Copy link
Contributor

@sulemanof sulemanof commented Jan 20, 2020

Summary

This includes a static generated file with peg parser and unit tests moved to jest from the legacy timelion app.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@sulemanof sulemanof added release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0 labels Jan 20, 2020
flash1293
flash1293 reviewed Jan 20, 2020
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Can you add documentation in a readme or something on how to create the parser code?

alexwizp
alexwizp reviewed Jan 21, 2020
View reviewed changes
...legacy/core_plugins/vis_type_timelion/public/components/timelion_expression_input_helpers.ts Outdated

// @ts-ignore
import grammar from 'raw-loader!../chain.peg';
import { parse } from '../chain';
Copy link
Contributor

@alexwizp alexwizp Jan 21, 2020

Choose a reason for hiding this comment

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

I prefer to mark somehow that this file was automatically generated from PEG, for example. for kuery.peg we moved it to the _generated_ folder

@alexwizp
Copy link
Contributor

alexwizp commented Jan 21, 2020

@flash1293 @sulemanof

  • should we add this file to .eslintignore?
    e.g. /src/legacy/core_plugins/vis_type_timelion/public/_generated_/**
  • also probably it should be added into our build process. See tasks/config/peg.js

alexwizp
alexwizp reviewed Jan 21, 2020
View reviewed changes
Copy link
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

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

See comments above

@sulemanof
Copy link
Contributor Author

sulemanof commented Jan 21, 2020

@flash1293 @sulemanof

  • should we add this file to .eslintignore?
    e.g. /src/legacy/core_plugins/vis_type_timelion/public/_generated_/**
  • also probably it should be added into our build process. See tasks/config/peg.js

Hey @alexwizp, your comments makes sense. Updated.
(but I'm not sure this task is somehow included into our build process, I suppose it's created for manual usage. Сorrect me, if I'm wrong)

@alexwizp
Copy link
Contributor

alexwizp commented Jan 21, 2020

LGTM! thanks

flash1293
flash1293 approved these changes Jan 21, 2020
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM, didn't check out locally

@kibanamachine
Copy link
Contributor

kibanamachine commented Jan 21, 2020

💚 Build Succeeded

History

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

@sulemanof sulemanof merged commit 6feabcd into elastic:master Jan 21, 2020
@sulemanof sulemanof deleted the tests/vis_type_timelion branch January 21, 2020 14:38
@sulemanof sulemanof mentioned this pull request Jan 21, 2020
Merged
sulemanof added a commit that referenced this pull request Jan 22, 2020
@sulemanof
Generate a static parser, move tests to vis_type_timelion (#55299) (#…
2339e28 
…55401)

* Use generated parser, move tests to vis_type_timelion

* Remove legacy tests

* Create a grunt task for generating a parser
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@flash1293 flash1293 flash1293 approved these changes

@alexwizp alexwizp alexwizp approved these changes

Assignees

No one assigned

Labels

backported release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sulemanof @alexwizp @kibanamachine @flash1293