seed icon indicating copy to clipboard operation
seed copied to clipboard

Before running the sketch, check if all references are valid

Open fdb opened this issue 7 years ago • 11 comments

Currently resolving references happens at runtime, which means sketches like this only fail 50% of the time:

root:
- This line is fine
- This line contains an invalid {{ reference }}

To avoid these errors happening at runtime I suggest creating a validation step in parsePhraseBook that checks if all references in the script are valid.

fdb avatar Feb 19 '18 16:02 fdb

This would require another loop to be run after the already running loop, because once the first loop is completed then only we can determine whether a reference is defined or not. If I am going in the right direction, can I try this?

kunal-mohta avatar Feb 19 '18 16:02 kunal-mohta

Actually we just need to check all references we encounter during parsing. We don't need to run the code "normally" at all.

Give it a try if you want!

fdb avatar Feb 20 '18 16:02 fdb

Okay, I will give it a try.

kunal-mohta avatar Feb 20 '18 17:02 kunal-mohta

Wait, I don't think I got this right. Say for example, we have

root:
- This is {{ reference1 }} //(*)
- This is {{ reference2 }} //(**)

//(***)
reference1:
- Some stuff

reference3:
- Some stuff

How will we know at line (*) and (**) that reference1 and reference2 are defined after (***) or not, without running the code "normally"

kunal-mohta avatar Feb 21 '18 06:02 kunal-mohta

Because before running the code we already have the phrase book, and a syntax tree through which all of the code runs. After parsing the code, the syntax tree needs to be traversed and every time a key is encountered (node.type === NODE_KEY) it should be checked against the phrase book. One caveat though, this key could also refer to an argument that has been passed, or an import key.

2018-02-21 7:02 GMT+01:00 k-cap [email protected]:

Wait, I don't think I got this right. Say for example, we have

root:

  • This is {{ reference1 }} //(*)
  • This is {{ reference2 }} //(**)

//(***) reference1:

  • Some stuff

reference3:

  • Some stuff

How will we know at line () and () that reference1 and reference2 are defined after () or not, without running the code "normally"

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/nodebox/seed/issues/17#issuecomment-367222942, or mute the thread https://github.com/notifications/unsubscribe-auth/AADAvgPZ8ch4GrDdGM8xhLtBwC4gR7PUks5tW7F6gaJpZM4SKxjQ .

stebanos avatar Feb 21 '18 06:02 stebanos

Oh! Okay. I thought you were planning to implement this inside parsePharsebook() function.

kunal-mohta avatar Feb 21 '18 07:02 kunal-mohta

Should the check be included here

kunal-mohta avatar Feb 21 '18 19:02 kunal-mohta

No idea. @stebanos ?

fdb avatar Feb 22 '18 19:02 fdb

No, it should be checked after everything is parsed, before returning the phraseBook, so yes, inside the parsePhrasebook function, but at the end. Because it is only at this point we know for sure what all the phrases and references are.

stebanos avatar Feb 22 '18 20:02 stebanos

Won't that mean rewriting the code that extracts the references from {{ ... }}, that already has been written?

kunal-mohta avatar Feb 22 '18 20:02 kunal-mohta

No, the check needs to perform a similar action as what happens in the Interpreter class. For each phrase, the Parser class returns a tree of nodes of a certain type, some of these nodes are of type NODE_KEY, it's these ones that need to be checked against the whole of the phraseBook. Since these nodes are "buried" within the tree, the tree needs to be traversed, like what happens in the visit(node) function.

stebanos avatar Feb 22 '18 20:02 stebanos