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

Create grunt task to generate parsers from peg files #17069

Merged
merged 3 commits into from
Mar 21, 2018

Conversation

lukasolson
Copy link
Member

As of #17049, we are using the generated parsing modules instead of dynamically creating the parser for PEG grammars.

Creating the PEG modules is an annoying task to run manually, especially with regards to keeping track of which options to use. This PR creates two grunt tasks: One for generating the parsing modules from the grammar (grunt peg) and another to watch the .peg files and then run the previous task (grunt watch:peg).

I had originally tried using the webpack loader for PEG but found out that this resulted in the same problem that was the reason we created #17049 to begin with... i.e. reporting would break presumably since Phantom would choke on the .peg files.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@lukasolson
Copy link
Member Author

Jenkins, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

tasks/peg.js Outdated
@@ -0,0 +1,24 @@
module.exports = function (grunt) {
const config = {
peg: {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should go in tasks/config/peg.js

tasks/peg.js Outdated
}
}
},
watch: {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should go in tasks/config/watch.js

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

We actually put the grunt config in files named after the top level key and merge them with load-grunt-config. Otherwise things look good

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.

Other than Spencer's comments the tasks lgtm.

Could you add a comment to the top of the .peg files explaining that these tasks need to be run before changes will take effect?

It would be cool to tie this into the optimizer and build process at some point so the generation would happen automatically, but I don't think it's really necessary in this PR.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Bargs
Copy link
Contributor

Bargs commented Mar 21, 2018

LGTM

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGTM

@lukasolson lukasolson merged commit 6bac181 into elastic:master Mar 21, 2018
lukasolson added a commit to lukasolson/kibana that referenced this pull request Mar 21, 2018
* Create grunt task to generate parsers from peg files

* Leave comment

* Move grunt config to config/
@lukasolson
Copy link
Member Author

6.x (6.3.0): 1224648

@lukasolson lukasolson deleted the pegTask branch March 27, 2018 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants