-
Notifications
You must be signed in to change notification settings - Fork 286
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
Parser changes for supporting relationship expiration #2141
Parser changes for supporting relationship expiration #2141
Conversation
This adds a concept of a "flaggable" lexer which, when detecting a `use (flag)` statement, runs a transformation to change the tokens returned to the parent. This is currently used to rewrite certain identifiers as keywords.
4de954b
to
9b89c57
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, had some questions/comments
@@ -11,6 +11,7 @@ const ( | |||
NodeTypeError NodeType = iota // error occurred; value is text of error | |||
NodeTypeFile // The file root node | |||
NodeTypeComment // A single or multiline comment | |||
NodeTypeUseFlag // A use flag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we worried about shifting the order of subsequent node types?
If not, I'll probably change some of how I'm doing my work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the order shouldn't matter here. All references are by names, and the tests all use the names too
{TokenTypeKeyword, 0, "expiration", ""}, | ||
tEOF, | ||
}}, | ||
{"use expiration and", "use expiration and", []Lexeme{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What role does and
play here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
relation viewer: user with caveat and expiration
- the and
is a keyword in this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to the new lexer; we no longer had a peek on it
if p.isToken(lexer.TokenTypeIdentifier) { | ||
useFlag, _ = p.consumeIdentifier() | ||
} else { | ||
useName, ok := p.consumeVariableKeyword() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea being that under some schema version, this is potentially already a keyword?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a keyword when the use
rule is in place, but not a keyword for invalid rules
@@ -35,7 +35,7 @@ NodeTypeFile | |||
caveat-name = somecaveat | |||
end-rune = 67 | |||
input-source = caveats type test | |||
start-rune = 53 | |||
start-rune = 58 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is kind of surprising - is this because with
was previously a part of the caveat definition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct
start-rune = 0 | ||
NodeTypeError | ||
end-rune = 22 | ||
error-message = Unexpected token at root level: TokenTypeIdentifier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an easy way to make this error more specific without special handling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not easily, no
Step 1 of support for first-class relationship expiration: parser changes