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

Implement parsing of local imports #2103

Merged
merged 1 commit into from
Oct 28, 2024
Merged

Implement parsing of local imports #2103

merged 1 commit into from
Oct 28, 2024

Conversation

tstirrat15
Copy link
Contributor

@tstirrat15 tstirrat15 commented Oct 25, 2024

Part of #1437

Description

A first step in implementing the composable schema proposal. This implements the relative import syntax in the parser.

Changes

  • Add a new test with the desired local import syntax
  • Add from and import as keywords
  • Add new import node type and predicates for it
  • Add parser logic to consume import syntax
  • Add generated test output
  • Fix trivy usage so that we don't hit rate limits

Testing

Review. See that tests are green.

@github-actions github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Oct 25, 2024
@tstirrat15 tstirrat15 marked this pull request as ready for review October 25, 2024 18:37
@tstirrat15 tstirrat15 requested a review from a team as a code owner October 25, 2024 18:37
.github/workflows/security.yaml Show resolved Hide resolved
.github/workflows/security.yaml Outdated Show resolved Hide resolved
@josephschorr josephschorr self-requested a review October 27, 2024 21:02
.github/workflows/security.yaml Show resolved Hide resolved
@@ -485,6 +488,7 @@ func (p *sourceParser) tryConsumeArrowExpression() (AstNode, bool) {
return nil, false
}

// TODO: is this the time to do this?
Copy link
Member

Choose a reason for hiding this comment

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

yep!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm gonna do that in a follow-on PR

return importNode
}

segmentNode, ok := p.tryConsumeIdentifierLiteral()
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a try consume; it should be a consume, because a missing identifier here should be an error. Please add a test case for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going off of the implementation of this function, which does seem to use consume:

identifier, _ := p.consumeIdentifier()

I'll add a test case, though.

}
}

p.consumeKeyword("import")
Copy link
Member

Choose a reason for hiding this comment

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

Return here if the keyword was not found


// Consume alternating identifiers and commas until we reach the end of the import statement
for {
definitionNode, ok := p.tryConsumeIdentifierLiteral()
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, not a try consume because it is required

@@ -122,6 +122,7 @@ func TestParser(t *testing.T) {
{"arrow illegal operations test", "arrowillegalops"},
{"arrow illegal function test", "arrowillegalfunc"},
{"caveat with keyword parameter test", "caveatwithkeywordparam"},
{"local imports test", "localimport"},
Copy link
Member

Choose a reason for hiding this comment

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

add a bunch of error cases as well

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 questions

Comment on lines 10 to 16
child-node =>
NodeTypeError
end-rune = 30
error-message = Expected one of: [TokenTypeComma], found: TokenTypeKeyword
error-source = caveat
input-source = local imports with keyword in identifiers test
start-rune = 32
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why doesn't this line up with the error below?

Copy link
Member

Choose a reason for hiding this comment

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

This typically is a result of missing a finishNode call somewhere, but I don't see it. You'll need to trace to see why that is

Comment on lines 11 to 16
NodeTypeError
end-rune = 30
error-message = Expected one of: [TokenTypeComma], found: TokenTypeKeyword
error-source = definition
input-source = local imports with malformed identifiers set test
start-rune = 33
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same question here. Is this expected?

// NodeTypeImport
//
// TODO: still need to figure out what form this should take - full path? relative path?
NodeImportPredicateSource = "import-source"
Copy link
Member

Choose a reason for hiding this comment

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

Match whatever we do for definition name paths

Comment on lines 10 to 16
child-node =>
NodeTypeError
end-rune = 30
error-message = Expected one of: [TokenTypeComma], found: TokenTypeKeyword
error-source = caveat
input-source = local imports with keyword in identifiers test
start-rune = 32
Copy link
Member

Choose a reason for hiding this comment

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

This typically is a result of missing a finishNode call somewhere, but I don't see it. You'll need to trace to see why that is

Add import consumption

gofumpt

Add import and from keywords

Add some new nodes

Fix node connection

Add new test to list

Generate output associated with new test

Get rid of unused node types

Shift nodes to predicates

Gofumpt

Add github token to trivy command

Add comment note

Get rid of unnecessary flag'

Add more test cases

Make updates to consumption

Add test cases

Add boolean return to consumeIdentiferLiteral

Get rid of child node issue

Apply suggestions from code review
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 Oct 28, 2024
Merged via the queue into main with commit 98a8473 Oct 28, 2024
22 checks passed
@tstirrat15 tstirrat15 deleted the local-imports branch October 28, 2024 21:36
@github-actions github-actions bot locked and limited conversation to collaborators Oct 28, 2024
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