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

Merge default types into modules with the same name (query builder) #793

Merged

Conversation

CarsonF
Copy link
Collaborator

@CarsonF CarsonF commented Nov 28, 2023

Given the schema

module default {
  type User {
    required status: User::Status;
  }
}
module User {
  type Status extending enum<Active, Disabled>;
}

In EdgeQL we can do

insert User {
  status := User::Status.Active
}

However, with the query builder the User type was not flattened because there's a module named User.
So we had to do

e.insert(e.default.User, {
  status: e.User.Status.Active,
});

With this PR we can do

e.insert(e.User, {
  status: e.User.Status.Active,
});

Which more closely matches EdgeQL

@CarsonF CarsonF force-pushed the merge-module-with-type-of-same-name branch from 3342574 to 26eb664 Compare November 28, 2023 19:59
@scotttrinh
Copy link
Collaborator

Do you mind adding a quick test case to the LTS schema here? You can add a new dummy type and dummy module with a type and add a low-effort test to the select tests there to ensure all of the type inference works out? If not I can stick it in my todo list.

@CarsonF
Copy link
Collaborator Author

CarsonF commented Nov 28, 2023

Absolutely, and thanks for the starting point. Wanted to get the concept in front of you before putting more work in.

@@ -122,7 +127,10 @@ export function generateIndex(params: GeneratorParams) {
allowFileExt: true,
});

index.writeln([r`${genutil.quote(moduleName)}: _${internalName},`]);
const valueStr = defaultSpreadTypes.has(moduleName)
? `{ ..._${internalName}, ..._default.${moduleName} }`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should this be frozen? Following suit of

return Object.freeze(expr) as any;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, good question. I assume the expression at internalName is indeed a frozen object, so it would make sense to match there. Yeah, let's do that, the tests should catch any weird regressions there since we have that new User module from the test commit.

@scotttrinh scotttrinh merged commit 90351a2 into edgedb:master Dec 1, 2023
7 checks passed
@CarsonF CarsonF deleted the merge-module-with-type-of-same-name branch December 1, 2023 18:59
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.

2 participants