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

Use 'Omit' instead of 'Pick<Exclude<...>>' for object rest #31134

Merged
merged 5 commits into from Apr 30, 2019
Merged

Use 'Omit' instead of 'Pick<Exclude<...>>' for object rest #31134

merged 5 commits into from Apr 30, 2019

Conversation

rpgeeganage
Copy link
Contributor

@rpgeeganage rpgeeganage commented Apr 27, 2019

Trying to deliver the Modification mentioned in #30948

Fixes #30948

@rpgeeganage rpgeeganage changed the title add Omit<T, ..> instead of Pick<Exclue<T>,..> Use 'Omit' instead of 'Pick<Exclude<...>>' for object rest Apr 27, 2019
@rpgeeganage
Copy link
Contributor Author

@sandersn ,
This is the PR for #30948.
Please give me feedback, regarding the modification.
Thank you.

@DanielRosenwasser
Copy link
Member

Seems like the change is probably good. Now run gulp baseline-accept to update the test outputs.

@rpgeeganage
Copy link
Contributor Author

@DanielRosenwasser ,
Thanks a lot for the feedback. I'll run it and update PR.

@DanielRosenwasser
Copy link
Member

Actually, I don't know if the change is appropriate because it will error if Omit isn't present. It's not clear if it should. There is a getGlobalTypeOrUndefined.

@DanielRosenwasser
Copy link
Member

Mm, team seems to think that maybe we shouldn't even have the fallback.

@rpgeeganage
Copy link
Contributor Author

@DanielRosenwasser ,
I'm sorry that I'm not clear about your comment. Can you please elaborate little bit if possible?

@rpgeeganage
Copy link
Contributor Author

rpgeeganage commented Apr 29, 2019

Ok, I can modify the code as follows.

if (! omitTypeAlias) {
  return errorType;
}

return getTypeAliasInstantiation(omitTypeAlias, [source, omitKeyType]);

and get rid of the code from line 4949 to 4955.

@msftclas
Copy link

msftclas commented Apr 29, 2019

CLA assistant check
All CLA requirements met.

@rpgeeganage
Copy link
Contributor Author

rpgeeganage commented Apr 29, 2019

@DanielRosenwasser ,
I changed the code. Can you please give me an insight on how can I fix the tests?
A sample error message is as follows.

The baseline file genericObjectRest.types has changed.
Error: The baseline file genericObjectRest.types has changed

@rpgeeganage
Copy link
Contributor Author

@DanielRosenwasser ,
Thanks. I fixed the tests\baselines\reference.

@RyanCavanaugh
Copy link
Member

@typescript-bot test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 30, 2019

Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at cf15c79. You can monitor the build here. It should now contribute to this PR's status checks.

@RyanCavanaugh RyanCavanaugh self-assigned this Apr 30, 2019
@RyanCavanaugh RyanCavanaugh merged commit 0c9a35c into microsoft:master Apr 30, 2019
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.

Use 'Omit' instead of 'Pick<Exclude<...>>' for object rest
5 participants