- From: Domenic Denicola <notifications@github.com>
- Date: Wed, 11 Oct 2017 01:00:04 +0000 (UTC)
- To: heycam/webidl <webidl@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <heycam/webidl/pull/458/review/68468125@github.com>
domenic approved this pull request. 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; Nit: `"use strict";` > @@ -0,0 +1,48 @@ +const Grammar = require("syntax-cli").Grammar; +const LLParsingTable = require("syntax-cli").LLParsingTable; +const jsdom = require("jsdom"); + +function getRulesFromDOM(window) { + let rules = window.document.querySelectorAll("pre.grammar[id]"); + return [].map.call(rules, pre => pre.textContent); +} + +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 => { Nit: two spaces after `return` seems silly. > + if (/^(integer|float|identifier|string|whitespace|comment|other)$/.test(m)) { + return m.toUpperCase(); + } + if (/:\n/.test(m)) { return "\n : "; } + if (/\n/.test(m)) { return "\n | "; } + if (/ε/.test(m)) { return "/* epsilon */"; } + }) + "\n ;"; + }); +} + +function toBNF(rules) { + return "\n%%\n\n" + processRules(rules).join("\n"); +} + +let path = process.argv[2]; +let html = require("fs").readFileSync(path, "utf8"); Nit: seems nicer to put all `require()`s at the top > @@ -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 $? 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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/heycam/webidl/pull/458#pullrequestreview-68468125
Received on Wednesday, 11 October 2017 01:00:27 UTC