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(types): Fixes array over union sub-return-types. #31

Merged
merged 5 commits into from
Jan 25, 2019

Conversation

nl-brett-stime
Copy link
Contributor

Wraps the ${first} return type in parentheses so that array type augments the whole sub-type e.g., in case the sub-type is a union.

E.g.,

From: mget(...keys: string[]): Promise<string | null[]>;   // string | Array<null>
  to: mget(...keys: string[]): Promise<(string | null)[]>; // Array<string | null>

From: hmget(key: string, ...fields: string[]): Promise<string | null[]>;   // string | Array<null>
  to: hmget(key: string, ...fields: string[]): Promise<(string | null)[]>; // Array<string | null>

Wraps the ${first} return type in parentheses so that array type augments the whole sub-type e.g., in case the sub-type is a union.

E.g.,
```
From: mget(...keys: string[]): Promise<string | null[]>;   // string | Array<null>
  to: mget(...keys: string[]): Promise<(string | null)[]>; // Array<string | null>

From: hmget(key: string, ...fields: string[]): Promise<string | null[]>;   // string | Array<null>
  to: hmget(key: string, ...fields: string[]): Promise<(string | null)[]>; // Array<string | null>
```
@nl-brett-stime
Copy link
Contributor Author

All of the CI tests pass but it's failing on a check for git status --porcelain, expecting empty string. It's failing because the PR has a modified file (by definition?). I'm not sure what, if anything, I'm supposed to change to fix this issue.

Copy link
Owner

@mmkal mmkal left a comment

Choose a reason for hiding this comment

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

Thanks for this, good catch! The git status --porcelain is there because since #24 the generated code is checked in, so we need to make sure that the code in git matches what the scripts generate.

If you run npm run ci locally on your branch, the generated file will be updated and the script will fail like on Travis. You can then commit that file (you should see the effect of your changes in the diff), and run npm run ci again. The second time it should succeed after detecting no more changes.

Let me know if this works for you. I'll add some docs around this to the readme.

@@ -249,7 +249,7 @@ const getReturnType = (sampleValues: any[] | undefined): string => {
const itemReturnTypes = new Set(sampleValues.map(getReturnType));
if (itemReturnTypes.size === 1) {
const first = itemReturnTypes.values().next().value;
return `${first}[]`;
return `(${first})[]`;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
return `(${first})[]`;
return `Array<${first}>`;

@mmkal mmkal changed the title Fixes array over union sub-return-types. fix(types): Fixes array over union sub-return-types. Jan 24, 2019
Adds generated code for CI guard
Reduces noise in generated types via extra check for pipe character so that parens are only added when the generated array sub-type is actually a union type. This seems to work fine for the current data types. Hopefully we won't ever need a parser to actually check for balanced parens various expressions before adding more.
@nl-brett-stime
Copy link
Contributor Author

Okay, I think I've got things straightened out after giving GitHub and Travis a workout. Might want to squash the commits in this PR if you can.

Thanks for the lib!

@mmkal
Copy link
Owner

mmkal commented Jan 25, 2019

@nl-brett-stime pushed a change only using the Array<...> format for complex types to reduce noise, along the same lines as one of your intermediate commits. Thanks again for the PR!

@mmkal mmkal merged commit 37b52f9 into mmkal:master Jan 25, 2019
@mmkal
Copy link
Owner

mmkal commented Jan 25, 2019

🎉 This PR is included in version 1.5.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

None yet

3 participants