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

fix: do not treat subpackage paths as part of the current package #1530

Merged
merged 3 commits into from
May 20, 2024

Conversation

matt2e
Copy link
Contributor

@matt2e matt2e commented May 17, 2024

fixes #1529

@matt2e matt2e requested review from alecthomas and a team as code owners May 17, 2024 06:20
@matt2e matt2e requested review from wesbillman and removed request for a team May 17, 2024 06:20
@ftl-robot ftl-robot mentioned this pull request May 17, 2024
@@ -398,46 +398,46 @@ func TestErrorReporting(t *testing.T) {
filename := filepath.Join(pwd, `testdata/failing/failing.go`)
actual := slices.Map(r.MustGet().Errors, func(e *schema.Error) string { return strings.TrimPrefix(e.Error(), filename+":") })
expected := []string{
`10:13-34: first argument to config and secret declarations must be the name as a string literal`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the new import has moved everything down a line...

@matt2e matt2e marked this pull request as draft May 17, 2024 06:22
@matt2e matt2e force-pushed the matt2e/improve-path-in-pkg branch from 7cc2796 to 81188b6 Compare May 17, 2024 06:46
`153:2-2: unsupported type ftl/failing/child.ChildString from subpackage`,
`153:2-8: unsupported type "ftl/failing/child.ChildString" for field "String"`,
`157:1-1: unsupported type ftl/failing/child.ChildStruct from subpackage`,
`157:1-33: unsupported type "struct{Name string}" for type alias`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

errors for line 152 onwards are what are actually being surfaced with the new test change

@matt2e matt2e force-pushed the matt2e/improve-path-in-pkg branch from 81188b6 to 8602e70 Compare May 17, 2024 06:54
@matt2e
Copy link
Contributor Author

matt2e commented May 17, 2024

This wasn't as neat as I'd hoped. Originally I was hoping it'd just be a change to isPathInPkg(path), but ended up needing additional checks each time. Otherwise it was unable to tell external refs (allowed) from subpackage refs (not allowed) because subpackages will naturally also start with ftl/

@matt2e matt2e marked this pull request as ready for review May 17, 2024 07:00
func (p *parseContext) isPathInPkg(path string) bool {
if path == p.pkg.PkgPath {
return true
if strings.Contains(path, "namedext") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Debugging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️

# Conflicts:
#	go-runtime/compile/schema_test.go
#	go-runtime/compile/testdata/failing/failing.go
# Conflicts:
#	go-runtime/compile/schema_test.go
@matt2e matt2e force-pushed the matt2e/improve-path-in-pkg branch from 81974a3 to f01b5fa Compare May 20, 2024 01:03
@matt2e matt2e merged commit 3f74d1c into main May 20, 2024
24 checks passed
@matt2e matt2e deleted the matt2e/improve-path-in-pkg branch May 20, 2024 01:06
matt2e added a commit that referenced this pull request May 22, 2024
…kage (#1530)"

This reverts commit 3f74d1c.

# Conflicts:
#	go-runtime/compile/schema.go
#	go-runtime/compile/schema_test.go
#	go-runtime/compile/testdata/failing/failing.go
matt2e added a commit that referenced this pull request May 23, 2024
Reverts two PRs:
- #1530
- #1535

This is a blunt fix to unblock based on our priorities. We should
revisit #1535 again later
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

types defined in subpackages are treated as valid types
2 participants