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

Handle >2 union members in links #669

Merged
merged 6 commits into from
Jul 11, 2023
Merged

Handle >2 union members in links #669

merged 6 commits into from
Jul 11, 2023

Conversation

scotttrinh
Copy link
Collaborator

@scotttrinh scotttrinh commented Jun 26, 2023

We do not need to generate a separate object type for a type that is a union of other types since we will have already generated the individual types and the use of that type within another type will be handled by the declaration of the consumer of the union.

TODO:

  • Add 2.x back to the testing matrix
  • Get 2.x passing
  • Test (and handle?) aliases that are unions
    • You cannot make an alias union type. There are ways for an alias to have a type that is a union (making it a computed set of each type), but the reflection query is likely the same, so I'll consider this fine for now.

@@ -158,93 +158,15 @@ export const getStringRepresentation: (
export const generateObjectTypes = (params: GeneratorParams) => {
const { dir, types } = params;

// const plainTypesCode = dir.getPath("types");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I'm just killing all this commented out code: it's already drifting pretty far, so I don't think it makes much sense to keep it.

continue;
}

const isUnionType = Boolean(type.union_of?.length);
const isIntersectionType = Boolean(type.intersection_of?.length);

if (isIntersectionType) {
if (isIntersectionType || isUnionType) {
Copy link
Collaborator Author

@scotttrinh scotttrinh Jun 26, 2023

Choose a reason for hiding this comment

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

Here's the actual fix: we just skip generating any type if it's actually a union instead of a separate type.

@@ -274,7 +274,7 @@ SELECT (SELECT __param__test)`
tuple: args,
});

assert.deepEqual(Object.values(complexResult.tuple), Object.values(args));
assert.deepEqual(complexResult.tuple, args);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This changed for no good reason, but our macos tests were failing with a test timeout until I did this update. 🫠

@scotttrinh scotttrinh changed the base branch from master to test-multiple-versions June 28, 2023 15:17
@scotttrinh scotttrinh marked this pull request as ready for review June 28, 2023 15:21
Base automatically changed from test-multiple-versions to master July 11, 2023 17:33
@scotttrinh scotttrinh requested review from jaclarke and removed request for jaclarke July 11, 2023 19:07
@scotttrinh scotttrinh merged commit afe8a0c into master Jul 11, 2023
8 checks passed
@scotttrinh scotttrinh deleted the 646-many-union-members branch July 11, 2023 19:09
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.

1 participant