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

Check grammar is LL(1) as part of Travis builds #458

Merged
merged 2 commits into from
Oct 11, 2017

Conversation

tobie
Copy link
Collaborator

@tobie tobie commented Oct 10, 2017

@tobie tobie requested a review from domenic October 10, 2017 20:56
@tobie tobie mentioned this pull request Oct 10, 2017
7 tasks
@tobie tobie force-pushed the check-grammar branch 4 times, most recently from 377aead to bdf8d8e Compare October 10, 2017 21:49
@tobie tobie requested a review from heycam October 10, 2017 21:54
@tobie
Copy link
Collaborator Author

tobie commented Oct 10, 2017

Now that @DmitrySoshnikov merged DmitrySoshnikov/syntax#49, this runs properly in Travis.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

JS style nits (feel free to ignore them) and a somewhat substantial deploy script issue, but LGTM otherwise. Thanks for cleaning up my mistakes with the extended attributes business. LL(1) seems like a pain.

@@ -0,0 +1,48 @@
const Grammar = require("syntax-cli").Grammar;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: "use strict";

check-grammar.js Outdated
function processRules(rules) {
const REGEXP = /(\s*:\n\s*)|\b(integer|float|identifier|string|whitespace|comment|other)\b|(\s*\n\s*)|(ε)/g;
return rules.map(rule => {
return rule.trim().replace(REGEXP, m => {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: two spaces after return seems silly.

check-grammar.js Outdated
}

let path = process.argv[2];
let html = require("fs").readFileSync(path, "utf8");
Copy link
Member

Choose a reason for hiding this comment

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

Nit: seems nicer to put all require()s at the top

deploy.sh Outdated
@@ -13,7 +13,8 @@ if [[ "$TRAVIS_PULL_REQUEST" != "false" || "$TRAVIS_BRANCH" != "$SOURCE_BRANCH"
echo "Skipping deploy; just doing a build."
mkdir out
doCompile
exit 0
node ./check-grammar.js ./out/index.html
exit $?
Copy link
Member

Choose a reason for hiding this comment

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

Why only check on pull requests?

I'd just put this inside doCompile. No need for exit $? then since we have done set -e above.

@tobie
Copy link
Collaborator Author

tobie commented Oct 11, 2017

Best resource I found to truly understand how a LL(1) parser is supposed to work: https://courses.cs.washington.edu/courses/cse401/04sp/slides/03b-LL1-example.pdf.

@tobie tobie merged commit 76c5192 into whatwg:master Oct 11, 2017
@tobie tobie deleted the check-grammar branch October 11, 2017 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants