Add Grammar Include Support #457

LiamRiddell · 2023-08-07T21:02:45Z

Fixes #286

I've implemented a fix that prevents VS Code complaining about LF vs CLRF line endings. This uses the solution provided in this article and enforces LF line endings.

Implementation (2.0)

Revert the original commit for ohm-cli
Add a new grammar rule for include '...' in the ohm-js grammar
Introduce unit tests for the new include statement in the grammar
Implement visitor for Document, Includes, and Include rules
Implement optional options object for the ohm.grammar function in ohm-js
Add new option called fetchImport that is provided via optional options object (Add circuit breaker for preventing infinite loop includes)
Add default implementation for Node and Browser, utilising .node.js and .browser.js file type support (optional)

Thoughts

How does this work when bundling with CLI for TypeScript? I feel we should have two modes, resolve and passthrough mode.

pdubroy · 2023-08-16T09:52:17Z

Hi Liam,

Thanks for the putting in the work to prototype this! Adding support for imports is something we've thought about over the years, but have hesitated to implement. Mainly, my concern is — what happens if someone is trying to load the grammar via ohm.grammar(...) rather than using the CLI? And then, the question is how to handle imports when Ohm is running in Node, vs. the browser?

This is a feature that I'd be happy to add, but if we're going to do it, I'd like to make sure we think it through and implement it fully. If it's something you're up for, we could collaborate on it.

I'll leave some notes on #286 about some of the things I'd like to address in a complete implementation.

pdubroy · 2023-08-16T09:52:46Z

Whoops, didn't mean to close this PR. I think we keep it open and iterate on it.

LiamRiddell · 2023-08-16T10:36:51Z

Hi Liam,

Thanks for the putting in the work to prototype this! Adding support for imports is something we've thought about over the years, but have hesitated to implement. Mainly, my concern is — what happens if someone is trying to load the grammar via ohm.grammar(...) rather than using the CLI? And then, the question is how to handle imports when Ohm is running in Node, vs. the browser?

This is a feature that I'd be happy to add, but if we're going to do it, I'd like to make sure we think it through and implement it fully. If it's something you're up for, we could collaborate on it.

I'll leave some notes on #286 about some of the things I'd like to address in a complete implementation.

Hi Patrick,

I'd love to collaborate, this as you mentioned was just a fix to get it working for my use case but I was keen to bring it in language as opposed to a CLI-only option.

ohm.grammar(`
  include './foo.ohm'

  G {
    ...
  }
`, {
  fetchGrammar(relativeUrl) {
    return fs.readFileSync(relativeUrl, import.meta.url, 'utf-8');
  }
})

This is a good starting point. Which allows the user to provide the method of resolution of the imported grammar. However, I'd expect the most common implementation to be:

Node - Read the file from disk based on the relative.
Browser - Fetched from url based on the relative path.

Which I believe we could provide default implementations for e.g. defaultFetchImport.node.js and defaultFetchImport.browser.js.

Next steps

I'll add a new grammar rule for include '...' in the ohm-js grammar including unit tests
Implement a new options object for the ohm.grammar function.
Add a new options for fetchImport

We then can evaluate the above and see how this would play with TypeScript and then make a new iteration with new learnings.

This reverts commit 11e40ac.

LiamRiddell · 2023-08-16T16:12:50Z

@pdubroy I'm trying to get the grammar working with the existing unit tests. However, I'm having issues with the test setup.
A few of the test files are referencing files that do not exist. What am I missing here? Ignore the valid failing tests.

Test Steps

pnpm prebootstrap
pnpm run bootstrap
pnpm test

/workspaces/ohm/packages/ohm-js (main) $ pnpm test

> ohm-js@17.1.0 test /workspaces/ohm/packages/ohm-js
> ava && node test/_test-doc.js


  ✖ extras › extractExamples › empty Error thrown in test
  ✖ extras › extractExamples › grammar with no examples Error thrown in test
  ✖ extras › extractExamples › simple positive examples Error thrown in test
  ✖ extras › extractExamples › examples for default start rule Error thrown in test
  ✖ extras › extractExamples › whitespace in trailing example comments Error thrown in test
  ✖ extras › extractExamples › example comments - negative examples Error thrown in test
  ✖ extras › extractExamples › example comments - corner cases Error thrown in test
  ✖ extras › extractExamples › extracted examples Error thrown in test
  ✖ examples › math example Error thrown in test
  ✖ examples › viz example Error thrown in test
  ✖ examples › csv example 
  ✔ built-in-rules › case-insensitive matching (298ms)
  ✔ built-in-rules › applySyntactic - basics (186ms)
  ✔ built-in-rules › applySyntactic - space skipping (111ms)
  ✔ built-in-rules › applySyntactic with lookahead
  ✔ errors › match failure (384ms)
  ✔ errors › undeclared rules (357ms)
  ✔ errors › many expressions with nullable operands (348ms)
  ✔ errors › errors from ohm.grammar() (225ms)
  ✔ errors › unrecognized escape sequences (219ms)
  ✔ errors › failures are memoized (194ms)
  ✔ errors › multiple MatchResults from the same Matcher (185ms)
  ✔ errors › non-fluffy failures subsume fluffy failures, etc. (141ms)
  ✔ errors › memo recs that do not contain the necessary info are deleted properly
  ✔ errors › trailing space should not influence the result
  ✔ errors › method name displayed on abstract function failure
  ✔ errors › errors for Not-of-<PExpr>
  ✔ errors › complex match failure
  ✔ errors › wrongNumberOfArguments includes the interval
  ✔ findIndentation › findIndentationPoints
  ✔ incremental › basic incremental parsing (275ms)
  ✔ incremental › trickier incremental parsing (256ms)
  ✔ incremental › examinedLength - no LR (228ms)
  ✔ incremental › examinedLength - no LR, but non-monotonic (196ms)
  ✔ incremental › examinedLength - simple LR (191ms)
  ✔ incremental › examinedLength - complicated LR (172ms)
  ✔ incremental › rightmostFailureOffset - no LR (155ms)
  ✔ incremental › rightmostFailureOffset - simple LR (151ms)
  ✔ incremental › rightmostFailureOffset - complicated LR (127ms)
  ✔ incremental › matchLength (124ms)
  ✔ incremental › matchLength - complicated LR (108ms)
  ✔ incremental › binding offsets - lexical rules
  ✔ incremental › binding offsets - syntactic rules
  ✔ incremental › incremental parsing + attributes = incremental computation
  ✔ grammar › action dictionary templates (302ms)
  ✔ grammar › default start rule (221ms)
  ✔ grammar › grammar equality (108ms)
  ✔ input-stream › next() at end
  ✔ indentation-sensitive › all dedents must be consumed (131ms)
  ✔ indentation-sensitive › can't skip over an indent (111ms)
  ✔ indentation-sensitive › any consumes indent and dedent
  ✔ indentation-sensitive › incremental parsing fails
  ✔ indentation-sensitive › memoization
  ✔ indentation-sensitive › basic tracing
  ✔ interval › collapsedLeft
  ✔ interval › collapsedRight
  ✔ interval › coverage - one interval
  ✔ interval › two adjacent intervals
  ✔ interval › coverage - two non-adjacent intervals
  ✔ interval › coverage - nested intervals
  ✔ interval › coverage - more intervals
  ✔ interval › brotha from anotha motha
  ✔ interval › coverageWith
  ✔ interval › getLineAndColumn
  ✔ main › namespaces (185ms)
  ✔ main › plain JS objects as namespaces (137ms)
  ✔ main › instantiating grammars from different types of objects
  ✔ parameterized-rules › require same number of params when overriding and extending (323ms)
  ✔ parameterized-rules › require same number of args when applying (244ms)
  ✔ parameterized-rules › require arguments to have arity 1 (194ms)
  ✔ parameterized-rules › require the rules referenced in arguments to be declared (184ms)
  ✔ parameterized-rules › simple examples (161ms)
  ✔ parameterized-rules › start matching from parameterized rule (123ms)
  ✔ parameterized-rules › inline rule declarations
  ✔ parameterized-rules › left recursion
  ✔ parameterized-rules › complex parameters
  ✔ parameterized-rules › duplicate parameter names
  ✔ ohm-syntax › char (906ms)
  ✔ ohm-syntax › string (882ms)
  ✔ ohm-syntax › unicode code point escapes (849ms)
  ✔ ohm-syntax › unicode recognition (778ms)
  ✔ ohm-syntax › unicode semantic actions (769ms)
  ✔ ohm-syntax › ranges (768ms)
  ✔ ohm-syntax › ranges w/ code points > 0xFFFF (738ms)
  ✔ ohm-syntax › ranges w/ code points > 0xFFFF, special cases (724ms)
  ✔ ohm-syntax › any consumes an entire code point (717ms)
  ✔ ohm-syntax › alt recognition (707ms)
  ✔ ohm-syntax › alt semantic actions (707ms)
  ✔ ohm-syntax › rule bodies in defs can start with a |, and it's a no-op recognition (706ms)
  ✔ ohm-syntax › rule bodies in defs can start with a |, and it's a no-op semantic actions (706ms)
  ✔ ohm-syntax › rule bodies in overrides can start with a |, and it's a no-op recognition (706ms)
  ✔ ohm-syntax › rule bodies in overrides can start with a |, and it's a no-op semantic actions (706ms)
  ✔ ohm-syntax › rule bodies in extends can start with a |, and it's a no-op recognition (706ms)
  ✔ ohm-syntax › rule bodies in extends can start with a |, and it's a no-op semantic actions (705ms)
  ✔ ohm-syntax › seq recognition (705ms)
  ✔ ohm-syntax › seq semantic actions (705ms)
  ✔ ohm-syntax › alts and seqs together recognition (695ms)
  ✔ ohm-syntax › alts and seqs together semantic actions (695ms)
  ✔ ohm-syntax › kleene-* and kleene-+ recognition (693ms)
  ✔ ohm-syntax › kleene-* and kleene-+ semantic actions (693ms)
  ✔ ohm-syntax › kleene-* and kleene-+ semantic actions are evaluated lazily (675ms)
  ✔ ohm-syntax › opt recognition (674ms)
  ✔ ohm-syntax › opt semantic actions (673ms)
  ✔ ohm-syntax › not recognition (673ms)
  ✔ ohm-syntax › not semantic actions (673ms)
  ✔ ohm-syntax › lookahead recognition (672ms)
  ✔ ohm-syntax › lookahead semantic actions (672ms)
  ✔ ohm-syntax › simple left recursion recognition (671ms)
  ✔ ohm-syntax › simple left recursion semantic actions (670ms)
  ✔ ohm-syntax › simple left recursion, with non-involved rules recognition (661ms)
  ✔ ohm-syntax › simple left recursion, with non-involved rules semantic actions (661ms)
  ✔ ohm-syntax › indirect left recursion recognition (660ms)
  ✔ ohm-syntax › indirect left recursion semantic actions (656ms)
  ✔ ohm-syntax › nested left recursion recognition (655ms)
  ✔ ohm-syntax › nested left recursion semantic actions (654ms)
  ✔ ohm-syntax › nested left recursion semantic actions are evaluated lazily (652ms)
  ✔ ohm-syntax › nested and indirect left recursion recognition (643ms)
  ✔ ohm-syntax › nested and indirect left recursion semantic actions (642ms)
  ✔ ohm-syntax › tricky left recursion (different heads at same position) recognition (641ms)
  ✔ ohm-syntax › tricky left recursion (different heads at same position) semantic actions (641ms)
  ✔ ohm-syntax › no namespace (640ms)
  ✔ ohm-syntax › empty namespace (638ms)
  ✔ ohm-syntax › duplicate definition (637ms)
  ✔ ohm-syntax › override it checks that rule exists in super-grammar (620ms)
  ✔ ohm-syntax › override shouldn't matter if arities aren't the same (618ms)
  ✔ ohm-syntax › override should be ok to add new cases (612ms)
  ✔ ohm-syntax › override recognition (610ms)
  ✔ ohm-syntax › override semantic actions (609ms)
  ✔ ohm-syntax › extend recognition (609ms)
  ✔ ohm-syntax › extend semantic actions (608ms)
  ✔ ohm-syntax › extend should check that rule exists in super-grammar (608ms)
  ✔ ohm-syntax › extend should make sure rule arities are compatible (606ms)
  ✔ ohm-syntax › extend should be ok to add new cases (597ms)
  ✔ ohm-syntax › override with "..." (595ms)
  ✔ ohm-syntax › bindings inconsistent arity in alts is an error (544ms)
  ✔ ohm-syntax › bindings by default, bindings are evaluated lazily (541ms)
  ✔ ohm-syntax › inline rule declarations (532ms)
  ✔ ohm-syntax › lexical vs. syntactic rules can't call syntactic rule from lexical rule, not not the other way around (473ms)
  ✔ ohm-syntax › lexical vs. syntactic rules lexical rules don't skip spaces implicitly (465ms)
  ✔ ohm-syntax › lexical vs. syntactic rules syntactic rules skip spaces implicitly (443ms)
  ✔ ohm-syntax › lexical vs. syntactic rules mixing lexical and syntactic rules works as expected (442ms)
  ✔ ohm-syntax › lexical vs. syntactic rules lexification operator works as expected (439ms)
  ✔ ohm-syntax › space skipping semantics (423ms)
  ✔ ohm-syntax › single-line comment after case name (#282) (419ms)
  ✔ ohm-syntax › bootstrap it can recognize arithmetic grammar (405ms)
  ✔ ohm-syntax › bootstrap it can recognize itself (398ms)
  ✔ ohm-syntax › bootstrap it can produce a grammar that works (320ms)
  ✔ ohm-syntax › bootstrap full bootstrap! (180ms)
  ✔ pexprs › rule definition source (175ms)
  ✔ pexprs › rule application source (144ms)
  ✔ pexprs › toDisplayString (103ms)
  ✔ pexprs › toString
  ✔ pexprs › toArgumentNameList
  ✔ recipes › simple grammar recipes (224ms)
  ✔ recipes › grammar recipes with supergrammars (205ms)
  ✔ recipes › grammar recipes involving parameterized rules (144ms)
  ✔ recipes › grammar recipes with source (116ms)
  ✔ recipes › recipes with U+2028 LINE SEPARATOR and U+2029 PARAGRAPH SEPARATOR
  ✔ recipes › semantics recipes
  ✔ recipes › semantics recipes (special cases)
  ✔ recipes › semantics recipes with extensions
  ✔ recipes › semantics recipes w/ method shorthand
  ✔ recipes › recipes with astral plane code units
  ✔ semantics › operations (451ms)
  ✔ semantics › operations with arguments (350ms)
  ✔ semantics › attributes (259ms)
  ✔ semantics › attributes - same-named attributes don't collide (230ms)
  ✔ semantics › semantics (218ms)
  ✔ semantics › _iter nodes (129ms)
  ✔ semantics › _terminal nodes (122ms)
  ✔ semantics › semantic action arity checks (113ms)
  ✔ semantics › extending semantics
  ✔ semantics › mixing nodes from one grammar with semantics from another
  ✔ semantics › asIteration
  ✔ semantics › sourceString
  ✔ semantics › sourceString - issue #188
  ✔ semantics › sourceString - issue #204
  ✔ semantics › action call stacks
  ✔ semantics › error message for incorrect _iter or _nonterminal arity
  ✔ semantics › toString on an inner NodeWrapper - issue #416
  ✔ tracing › basic tracing (268ms)
  ✔ tracing › space skipping (252ms)
  ✔ tracing › tracing with parameterized rules (221ms)
  ✔ tracing › tracing with memoization (184ms)
  ✔ tracing › tracing with parameterized rules and memoization (163ms)
  ✔ tracing › tracing with left recursion (125ms)
  ✔ tracing › tracing with left recursion and leading space (108ms)
  ✔ tracing › toString
  ✔ tracing › toString with left recursion
  ✔ tracing › displayString
  ✔ tracing › memoization
  ✔ tracing › bindings
  ✔ tracing › tracing with "..."
  ✔ util › getLineAndColumn().toString()
  ✔ util › getLineAndColumnMessage
  ✔ util › getLineAndColumnMessage with ranges
  ✔ examples › prettyPrint › basic
  ✔ examples › prettyPrint › grammar with supergrammar
  ✔ examples › prettyPrint › multiple rules
  ✔ extras › storedAttributes › stored attributes
  ✔ extras › visitorFamily › basic
  ✔ extras › visitorFamily › array props
  ✔ extras › visitorFamily › arity checks
  ✔ extras › visitorFamily › unknown action names
  ✔ extras › visitorFamily › unrecognized tags
  ✔ extras › visitorFamily › operations with arguments
  ✔ extras › toAST › semantic action (189ms)
  ✔ extras › toAST › default (139ms)
  ✔ extras › toAST › mapping
  ✔ extras › toAST › real examples (combinations)
  ✔ extras › toAST › usage errors
  ✔ extras › toAST › listOf and friends - #394
  ─

  extras › extractExamples › empty

  test/extras/test-extractExamples.js:5

   4: test('empty', t => {                   
   5:   t.deepEqual(extractExamples(''), []);
   6: });                                    

  Error thrown in test:

  missingSemanticAction (Error) {
    message: `Missing semantic action for 'Document' in operation 'examples'.␊
    Action stack (most recent call last):␊
    ␊
      examples > Document`,
  }

  › examples > Document
  › createError (file://src/errors.js:15:9)
  › Module.missingSemanticAction (file://src/errors.js:305:13)
  › Semantics.Wrapper.<anonymous> (file://src/Semantics.js:491:20)
  › Operation.execute (file://src/Semantics.js:650:39)
  › Semantics.Wrapper.doIt [as examples] (file://src/Semantics.js:368:29)
  › extractExamples (file://extras/extractExamples.js:156:33)
  › file://test/extras/test-extractExamples.js:5:15



  extras › extractExamples › grammar with no examples

  test/extras/test-extractExamples.js:9

   8: test('grammar with no examples', t => {     
   9:   t.deepEqual(extractExamples('G { }'), []);
   10: });                                         

  Error thrown in test:

  missingSemanticAction (Error) {
    message: `Missing semantic action for 'Document' in operation 'examples'.␊
    Action stack (most recent call last):␊
    ␊
      examples > Document`,
  }

  › examples > Document
  › createError (file://src/errors.js:15:9)
  › Module.missingSemanticAction (file://src/errors.js:305:13)
  › Semantics.Wrapper.<anonymous> (file://src/Semantics.js:491:20)
  › Operation.execute (file://src/Semantics.js:650:39)
  › Semantics.Wrapper.doIt [as examples] (file://src/Semantics.js:368:29)
  › extractExamples (file://extras/extractExamples.js:156:33)
  › file://test/extras/test-extractExamples.js:9:15



  extras › extractExamples › simple positive examples

  test/extras/test-extractExamples.js:13

   12: test('simple positive examples', t => {
   13:   let examples = extractExamples(`     
   14:     G {                                

  Error thrown in test:

  missingSemanticAction (Error) {
    message: `Missing semantic action for 'Document' in operation 'examples'.␊
    Action stack (most recent call last):␊
    ␊
      examples > Document`,
  }

  › examples > Document
  › createError (file://src/errors.js:15:9)
  › Module.missingSemanticAction (file://src/errors.js:305:13)
  › Semantics.Wrapper.<anonymous> (file://src/Semantics.js:491:20)
  › Operation.execute (file://src/Semantics.js:650:39)
  › Semantics.Wrapper.doIt [as examples] (file://src/Semantics.js:368:29)
  › extractExamples (file://extras/extractExamples.js:156:33)
  › file://test/extras/test-extractExamples.js:13:18



  extras › extractExamples › examples for default start rule

  test/extras/test-extractExamples.js:37

   36: test('examples for default start rule', t => {
   37:   let examples = extractExamples(`            
   38:     G {                                       

  Error thrown in test:

  missingSemanticAction (Error) {
    message: `Missing semantic action for 'Document' in operation 'examples'.␊
    Action stack (most recent call last):␊
    ␊
      examples > Document`,
  }

  › examples > Document
  › createError (file://src/errors.js:15:9)
  › Module.missingSemanticAction (file://src/errors.js:305:13)
  › Semantics.Wrapper.<anonymous> (file://src/Semantics.js:491:20)
  › Operation.execute (file://src/Semantics.js:650:39)
  › Semantics.Wrapper.doIt [as examples] (file://src/Semantics.js:368:29)
  › extractExamples (file://extras/extractExamples.js:156:33)
  › file://test/extras/test-extractExamples.js:37:18



  extras › extractExamples › whitespace in trailing example comments

  test/extras/test-extractExamples.js:61

   60:   const expected = [{grammar: 'G', rule: '', example: '', shouldMatch: true}];
   61:   t.deepEqual(extractExamples('G{  //+ ""\n  }'), expected);                  
   62:   t.deepEqual(extractExamples('G{  //+ "" \n}'), expected);                   

  Error thrown in test:

  missingSemanticAction (Error) {
    message: `Missing semantic action for 'Document' in operation 'examples'.␊
    Action stack (most recent call last):␊
    ␊
      examples > Document`,
  }

  › examples > Document
  › createError (file://src/errors.js:15:9)
  › Module.missingSemanticAction (file://src/errors.js:305:13)
  › Semantics.Wrapper.<anonymous> (file://src/Semantics.js:491:20)
  › Operation.execute (file://src/Semantics.js:650:39)
  › Semantics.Wrapper.doIt [as examples] (file://src/Semantics.js:368:29)
  › extractExamples (file://extras/extractExamples.js:156:33)
  › file://test/extras/test-extractExamples.js:61:15



  extras › extractExamples › example comments - negative examples

  test/extras/test-extractExamples.js:67

   66: function getExamples(input) {                                                        
   67:   return extractExamples(`G { ${input}\nstart = }`).map(({example, shouldMatch}) => {
   68:     return {example, shouldMatch};                                                   

  Error thrown in test:

  missingSemanticAction (Error) {
    message: `Missing semantic action for 'Document' in operation 'examples'.␊
    Action stack (most recent call last):␊
    ␊
      examples > Document`,
  }

  › examples > Document
  › createError (file://src/errors.js:15:9)
  › Module.missingSemanticAction (file://src/errors.js:305:13)
  › Semantics.Wrapper.<anonymous> (file://src/Semantics.js:491:20)
  › Operation.execute (file://src/Semantics.js:650:39)
  › Semantics.Wrapper.doIt [as examples] (file://src/Semantics.js:368:29)
  › extractExamples (file://extras/extractExamples.js:156:33)
  › getExamples (file://test/extras/test-extractExamples.js:67:10)
  › file://test/extras/test-extractExamples.js:73:15



  extras › extractExamples › example comments - corner cases

  test/extras/test-extractExamples.js:67

   66: function getExamples(input) {                                                        
   67:   return extractExamples(`G { ${input}\nstart = }`).map(({example, shouldMatch}) => {
   68:     return {example, shouldMatch};                                                   

  Error thrown in test:

  missingSemanticAction (Error) {
    message: `Missing semantic action for 'Document' in operation 'examples'.␊
    Action stack (most recent call last):␊
    ␊
      examples > Document`,
  }

  › examples > Document
  › createError (file://src/errors.js:15:9)
  › Module.missingSemanticAction (file://src/errors.js:305:13)
  › Semantics.Wrapper.<anonymous> (file://src/Semantics.js:491:20)
  › Operation.execute (file://src/Semantics.js:650:39)
  › Semantics.Wrapper.doIt [as examples] (file://src/Semantics.js:368:29)
  › extractExamples (file://extras/extractExamples.js:156:33)
  › getExamples (file://test/extras/test-extractExamples.js:67:10)
  › file://test/extras/test-extractExamples.js:89:7



  extras › extractExamples › extracted examples

  test/extras/test-extractExamples.js:126

   125: test('extracted examples', t => {                                                       
   126:   for (const {grammar, rule, example, shouldMatch} of extractExamples(grammarsSource)) {
   127:     const g = grammars[grammar];                                                        

  Error thrown in test:

  missingSemanticAction (Error) {
    message: `Missing semantic action for 'Document' in operation 'examples'.␊
    Action stack (most recent call last):␊
    ␊
      examples > Document`,
  }

  › examples > Document
  › createError (file://src/errors.js:15:9)
  › Module.missingSemanticAction (file://src/errors.js:305:13)
  › Semantics.Wrapper.<anonymous> (file://src/Semantics.js:491:20)
  › Operation.execute (file://src/Semantics.js:650:39)
  › Semantics.Wrapper.doIt [as examples] (file://src/Semantics.js:368:29)
  › extractExamples (file://extras/extractExamples.js:156:33)
  › file://test/extras/test-extractExamples.js:126:55



  examples › math example

  test/test-examples.cjs:90

   89:   var srcDate = new Date(mtimes.pop());                                      
   90:   var bundleDate = fs.statSync(path.join(__dirname, '../dist/ohm.js')).mtime;
   91:                                                                              

  Error thrown in test:

  Error {
    code: 'ENOENT',
    errno: -2,
    path: '/workspaces/ohm/packages/ohm-js/dist/ohm.js',
    syscall: 'stat',
    message: 'ENOENT: no such file or directory, stat \'/workspaces/ohm/packages/ohm-js/dist/ohm.js\'',
  }

  › rebuildIfModified (test/test-examples.cjs:90:23)
  › test/test-examples.cjs:106:3



  examples › viz example

  test/test-examples.cjs:90

   89:   var srcDate = new Date(mtimes.pop());                                      
   90:   var bundleDate = fs.statSync(path.join(__dirname, '../dist/ohm.js')).mtime;
   91:                                                                              

  Error thrown in test:

  Error {
    code: 'ENOENT',
    errno: -2,
    path: '/workspaces/ohm/packages/ohm-js/dist/ohm.js',
    syscall: 'stat',
    message: 'ENOENT: no such file or directory, stat \'/workspaces/ohm/packages/ohm-js/dist/ohm.js\'',
  }

  › rebuildIfModified (test/test-examples.cjs:90:23)
  › test/test-examples.cjs:113:3



  examples › csv example

  test/test-examples.cjs:120

   119: test('csv example', function (t) {                      
   120:   t.notThrows(function () {                             
   121:     require(path.join(EXAMPLE_ROOT, 'csv', 'index.js'));

  Function threw:

  Error {
    code: 'MODULE_NOT_FOUND',
    path: '/workspaces/ohm/packages/ohm-js/package.json',
    requestPath: '../../packages/ohm-js',
    message: 'Cannot find module \'/workspaces/ohm/packages/ohm-js/dist/ohm.cjs\'. Please verify that the package.json has a valid "main" entry',
  }

  › Object.<anonymous> (/workspaces/ohm/examples/csv/index.js:8:13)

  ─

  11 tests failed
  2 known failures
 ELIFECYCLE  Test failed. See above for more details.
 ```

pdubroy · 2023-08-16T17:33:40Z

A couple of the tests depend on the generated bundles, so I think you need to run pnpm build before pnpm test.

(It's not ideal — maybe we should run it build automatically before the test, but that can result in wasted work in a lot of circumstances.)

packages/ohm-js/src/ohm-grammar.ohm

LiamRiddell · 2023-08-16T18:27:47Z

A couple of the tests depend on the generated bundles, so I think you need to run pnpm build before pnpm test.

(It's not ideal — maybe we should run it build automatically before the test, but that can result in wasted work in a lot of circumstances.)

That makes sense, I just ran a pnpm build followed by pnpm build and it's working as expected with all but two tests passing.

tracing › memoization
tracing › bindings

Do these usually fail?

pdubroy · 2023-08-16T18:31:37Z

Do these usually fail?

Yes, those are marked as known failures. The output is slightly confusing but ava allows you to mark tests as "known failures" so that pnpm test succeeds (= return code 0) but still reports those as known failures.

LiamRiddell · 2023-08-16T18:34:23Z

Do these usually fail?

Yes, those are marked as known failures. The output is slightly confusing but ava allows you to mark tests as "known failures" so that pnpm test succeeds (= return code 0) but still reports those as known failures.

Interesting, thanks for the insight. That makes sense to why I'm getting the return code 0. I'm looking at implementing unit tests now for the grammar modifications.

LiamRiddell · 2023-08-18T22:55:28Z

@pdubroy It would be good to do a partial review now. All tests are passing ✔️

Side notes:

I think it would make sense to look at bringing the namespace options into the options object I've created. However, let me know your thoughts?
I did some investigation, in the current setup I don't think it's easily possible to provide a default fetchGrammar implementation for node and the browser.

pdubroy

Looks like a great start! I have a few questions, and a request to change the implementation a bit.

packages/ohm-js/extras/extractExamples.js

pdubroy · 2023-08-26T08:25:15Z

packages/ohm-js/src/ohm-grammar.ohm

+  include
+  	= "include"
+
+  relativeFilePath 


What's the reason for choosing to allow these specific characters? Maybe I'm wrong, but it seems like it may be overly restrictive.

An alternative would be for Ohm to be completely agnostic about paths, and accept any character inside the quotes.

This is following security standards to always use an allowlist when you know the expected content format. If you want to accept the risk and open it to any character between the quotes we can do that.

packages/ohm-js/src/buildGrammar.js

pdubroy · 2023-08-26T08:43:43Z

packages/ohm-js/test/test-ohm-syntax.js

@@ -1382,22 +1382,22 @@ describe('bootstrap', test => {
  const ns = ohm.grammars(ohmGrammarSource);

  test('it can recognize arithmetic grammar', t => {
-    assertSucceeds(t, ns.Ohm.match(arithmeticGrammarSource, 'Grammar'));


Were these changes necessary, or is this an unrelated cleanup?

Since we added new Document rule to grammar the default rule is now Document, I feel the tests should be validating against the new grammar and not a sub-rule (e.g. Grammar) of the main grammar. Hence the change and resulting fixes to array accesses below.

LiamRiddell and others added 2 commits August 7, 2023 22:02

Added include support for ohm-cli

11e40ac

Fixed Typo

afb04cb

pdubroy closed this Aug 16, 2023

pdubroy reopened this Aug 16, 2023

pdubroy mentioned this pull request Aug 16, 2023

Grammar syntax for imports/includes #286

Open

LiamRiddell and others added 9 commits August 16, 2023 14:01

Enforcing LF line endings on windows

d64f080

Added support for include statement in the Ohm grammar

e3c3212

Revert "Added include support for ohm-cli"

920cadf

This reverts commit 11e40ac.

Removed redundant whitespace

004df2a

Reordered rules in grammar

50480e1

Renamed lexical rule

fb13e78

Removed redundant whitespace

f2b87d9

Added new Rule that encompasses Includes and Grammas into a "Document".

7d04b35

Matched naming of Grammars for Includes

3000e63

pdubroy reviewed Aug 16, 2023

View reviewed changes

packages/ohm-js/src/ohm-grammar.ohm Outdated Show resolved Hide resolved

LiamRiddell and others added 3 commits August 16, 2023 19:20

Remove case insensitive

0eda407

Removed case insensitive

7c293f7

Added document to the examples unit tests

4751dfc

LiamRiddell and others added 4 commits August 17, 2023 11:01

WIP Commit

4f2b480

Merge branch 'main' of https://github.com/LiamRiddell/ohm into main

7ef3fc0

Added unit tests

87732d4

Grammar

1e19398

Fixed tests due to Document changes

63ab3c6

pdubroy reviewed Aug 26, 2023

View reviewed changes

LiamRiddell added 2 commits August 26, 2023 10:15

Implemented review comments

2936708

Minor renaming

9073f4e

LiamRiddell requested a review from pdubroy August 26, 2023 10:19

LiamRiddell changed the title ~~Added include support for ohm-cli~~ Added include support Sep 9, 2023

LiamRiddell changed the title ~~Added include support~~ Add Grammar Include Support Sep 9, 2023

LiamRiddell closed this Oct 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Grammar Include Support #457

Add Grammar Include Support #457

LiamRiddell commented Aug 7, 2023 •

edited

pdubroy commented Aug 16, 2023

pdubroy commented Aug 16, 2023

LiamRiddell commented Aug 16, 2023 •

edited

LiamRiddell commented Aug 16, 2023

pdubroy commented Aug 16, 2023

LiamRiddell commented Aug 16, 2023

pdubroy commented Aug 16, 2023

LiamRiddell commented Aug 16, 2023 •

edited

LiamRiddell commented Aug 18, 2023 •

edited

pdubroy left a comment

pdubroy Aug 26, 2023

LiamRiddell Aug 26, 2023 •

edited

pdubroy Aug 26, 2023

LiamRiddell Aug 26, 2023

Add Grammar Include Support #457

Add Grammar Include Support #457

Conversation

LiamRiddell commented Aug 7, 2023 • edited

Implementation (2.0)

Thoughts

pdubroy commented Aug 16, 2023

pdubroy commented Aug 16, 2023

LiamRiddell commented Aug 16, 2023 • edited

Next steps

LiamRiddell commented Aug 16, 2023

Test Steps

pdubroy commented Aug 16, 2023

LiamRiddell commented Aug 16, 2023

pdubroy commented Aug 16, 2023

LiamRiddell commented Aug 16, 2023 • edited

LiamRiddell commented Aug 18, 2023 • edited

pdubroy left a comment

Choose a reason for hiding this comment

pdubroy Aug 26, 2023

Choose a reason for hiding this comment

LiamRiddell Aug 26, 2023 • edited

Choose a reason for hiding this comment

pdubroy Aug 26, 2023

Choose a reason for hiding this comment

LiamRiddell Aug 26, 2023

Choose a reason for hiding this comment

LiamRiddell commented Aug 7, 2023 •

edited

LiamRiddell commented Aug 16, 2023 •

edited

LiamRiddell commented Aug 16, 2023 •

edited

LiamRiddell commented Aug 18, 2023 •

edited

LiamRiddell Aug 26, 2023 •

edited