Conversation
tstirrat15
force-pushed
the
add-partial-parsing
branch
2 times, most recently
from
613af9b to
cf4a8c6
Compare
tstirrat15
force-pushed
the
add-partial-parsing
branch
from
cf4a8c6 to
ccf2722
Compare
tstirrat15
force-pushed
the
add-partial-parsing
branch
3 times, most recently
from
9c959e2 to
2650f8f
Compare
tstirrat15
left a comment
tstirrat15
left a comment
There was a problem hiding this comment.
See comments.
| ParsedBinaryExpression, | ||
| ParsedCaveatDefinition, | ||
| ParsedNamedArrowExpression, | ||
| ParsedNilExpression, | ||
| ParsedObjectDefinition, | ||
| ParsedRelationRefExpression, | ||
| ParsedUseFlag, |
There was a problem hiding this comment.
I got rid of all of the casts in this file, so the types are no longer necessary.
| const schema = `use expiration`; | ||
| const parsed = parseSchema(schema); | ||
|
|
||
| expect(parsed?.definitions).toHaveLength(1); |
There was a problem hiding this comment.
I changed most of the .length).toEqual()s to .toHaveLength()s - it's a better assertion for the context.
| assert(useFlag); | ||
| assert(useFlag.kind === "use"); |
There was a problem hiding this comment.
The use of assert here is what means that we don't need the ases anymore. It does the narrowing for us.
|
|
||
| describe("imports", () => { | ||
| it("parses basic import", () => { | ||
| const schema = `import "foo/bar/baz.zed"`; |
There was a problem hiding this comment.
Here's the tests for import syntax. Lemme know if there's other tests you'd want to see.
| }); | ||
| }); | ||
|
|
||
| describe("partial syntax", () => { |
There was a problem hiding this comment.
Partial tests are here. Lemme know if there are others you want to see.
| }, | ||
| ); | ||
| }); | ||
| const caveatParameterTypeExpr: Parser<ParsedCaveatParameterTypeRef> = lazy( |
There was a problem hiding this comment.
The only thing that changed here is the type on the parser, but prettier reordered things so it looks like more changed.
| .or(partial) | ||
| .or(caveat) | ||
| .or(useFlag) | ||
| .or(importExpression); |
There was a problem hiding this comment.
We added both partial and importExpression here as potential toplevels.
| const mapParseNodes = | ||
| (mapper: (node: ParsedNode) => void) => (node: ParsedNode | undefined) => { | ||
| if (node === undefined) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I left the contents of this mostly unchanged, but I changed the signature from being a function that takes a node and a mapper to a function that takes a mapper and returns a function that takes a node. This made the recursive calls a little nicer.
I'm not entirely sure that the mapper even really needs to be in there in the way it is; I may update that further later.
| case "partial": | ||
| node.relations.forEach(mapParseNodes(mapper)); | ||
| node.permissions.forEach(mapParseNodes(mapper)); | ||
| break; |
There was a problem hiding this comment.
We added this case to the switch.
|
|
||
| switch (node.kind) { | ||
| case "objectDef": | ||
| node.relations.forEach(mapParseNodes(mapper)); |
There was a problem hiding this comment.
This is the part that got a little nicer - mapParseNodes(mapper) can be handed directly to forEach.
tstirrat15
force-pushed
the
add-partial-parsing
branch
from
2650f8f to
f6c2bc5
Compare
tstirrat15
force-pushed
the
add-partial-parsing
branch
from
f6c2bc5 to
775ef72
Compare
1 participant
Description
Part of getting composable schema support into vscode.
Note
I haven't yet been able to test this against the extension because we need to update some part of SpiceDB (likely the LSP, but need to figure that out) before it's possible to test this. I'm pretty sure this code works, based on the tests and types, and I figure we can do the integration work when we get there.
Changes
Will annotate.
Testing
Review. Hide whitespace for a nicer diff, especially in the test file. See that tests pass.