Skip to content
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

Port in expiration parsing into composable schema DSL #2239

Merged
merged 1 commit into from
Feb 8, 2025

Conversation

tstirrat15
Copy link
Contributor

Description

The first step towards making use expiration and the expiration trait into the composable schema compiler. This mirrors the changes in #2141 into the composableschemadsl package.

Changes

  • Copy in new files from PR
  • Diff package and bring over changes
  • Reserve additional keywords

Testing

See that tests pass.

@tstirrat15 tstirrat15 requested a review from a team as a code owner February 7, 2025 23:46
@github-actions github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Feb 7, 2025
Copy link
Contributor Author

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments

Comment on lines +87 to +96
// Parking lot for future keywords
"and": {},
"or": {},
"not": {},
"under": {},
"static": {},
"if": {},
"where": {},
"private": {},
"public": {},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one change that wasn't a direct copy.

defer p.mustFinishNode()

// consume the `use`
p.consumeKeyword("use")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making this a keyword is one change from the schemadsl package.

@@ -35,7 +35,7 @@ NodeTypeFile
caveat-name = somecaveat
end-rune = 67
input-source = caveats type test
start-rune = 53
start-rune = 58
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a number of these changes where with is now on the other side of a node boundary. This results in the start-rune being incremented by 5, which is consistent with #2141.

@@ -135,8 +141,7 @@ func TestParser(t *testing.T) {
{"partials with malformed partial block", "partials_with_malformed_partial_block"},
}

for _, test := range parserTests {
test := test
for _, test := range parserTests { test := test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undo this change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@tstirrat15 tstirrat15 force-pushed the migrate-expiration-into-composable-schema branch from a87be96 to 90453da Compare February 7, 2025 23:50
@tstirrat15 tstirrat15 force-pushed the migrate-expiration-into-composable-schema branch from 90453da to 726aac4 Compare February 8, 2025 00:13
Copy link
Contributor Author

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment

Comment on lines +11 to +48
{"use expiration", "use expiration", []Lexeme{
{TokenTypeKeyword, 0, "use", ""},
{TokenTypeWhitespace, 0, " ", ""},
{TokenTypeKeyword, 0, "expiration", ""},
tEOF,
}},
{"use expiration and", "use expiration and", []Lexeme{
{TokenTypeKeyword, 0, "use", ""},
{TokenTypeWhitespace, 0, " ", ""},
{TokenTypeKeyword, 0, "expiration", ""},
{TokenTypeWhitespace, 0, " ", ""},
{TokenTypeKeyword, 0, "and", ""},
tEOF,
}},
{"expiration as non-keyword", "foo expiration", []Lexeme{
{TokenTypeIdentifier, 0, "foo", ""},
{TokenTypeWhitespace, 0, " ", ""},
{TokenTypeKeyword, 0, "expiration", ""},
tEOF,
}},
{"and as non-keyword", "foo and", []Lexeme{
{TokenTypeIdentifier, 0, "foo", ""},
{TokenTypeWhitespace, 0, " ", ""},
{TokenTypeKeyword, 0, "and", ""},
tEOF,
}},
{"invalid use flag", "use foobar", []Lexeme{
{TokenTypeKeyword, 0, "use", ""},
{TokenTypeWhitespace, 0, " ", ""},
{TokenTypeIdentifier, 0, "foobar", ""},
tEOF,
}},
{"use flag after definition", "definition use expiration", []Lexeme{
{TokenTypeKeyword, 0, "definition", ""},
{TokenTypeWhitespace, 0, " ", ""},
{TokenTypeKeyword, 0, "use", ""},
{TokenTypeWhitespace, 0, " ", ""},
{TokenTypeKeyword, 0, "expiration", ""},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth reworking these tests? use and expiration are now keywords, so the semantics of the tests don't completely scan.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think so; we should add them in as formal keywords in the lexer tests

Copy link
Member

@josephschorr josephschorr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tstirrat15 tstirrat15 added this pull request to the merge queue Feb 8, 2025
Merged via the queue into main with commit 5e7c223 Feb 8, 2025
39 checks passed
@tstirrat15 tstirrat15 deleted the migrate-expiration-into-composable-schema branch February 8, 2025 00:31
@github-actions github-actions bot locked and limited conversation to collaborators Feb 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants