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

Add the Omit helper type #30552

Merged
merged 4 commits into from
Apr 4, 2019
Merged

Add the Omit helper type #30552

merged 4 commits into from
Apr 4, 2019

Conversation

DanielRosenwasser
Copy link
Member

Fixes #30455.

@DanielRosenwasser
Copy link
Member Author

@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 22, 2019

Heya @DanielRosenwasser, I've started to run the Definitely Typed test suite on this PR at 53e76b3. You can monitor the build here. It should now contribute to this PR's status checks.

@DanielRosenwasser
Copy link
Member Author

The only breakages on DefinitelyTyped seem to be

  • amap-js-api
  • amap-js-api-indoor-map
  • amap-js-api-map3d

And those all really come from amap-js-api unnecessarily plopping Omit in the global scope.

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Apr 3, 2019

A fix is now out to DefinitelyTyped's amap-js-api package.

I used BigTSQuery to partially assess the breakage with the following query

SourceFile:not([externalModuleIndicator]) > TypeAliasDeclaration > Identifier[name=Omit]

BigTSQuery isn't fully up to date (@urish out of curiosity, how recent is the dataset?), but we saw three distinct results:

Also:

@DanielRosenwasser
Copy link
Member Author

We discussed this at #30738 and believe that the breakage can be easily communicated and is more limited than we initially anticipated.

@levenleven
Copy link

Keys are not limited to keyof T. Is that on purpose?

@vkrol
Copy link

vkrol commented Apr 7, 2019

@levenleven

Should we constrain K in the official definition?

Nah

#30738

🤔

@DanielRosenwasser
Copy link
Member Author

Exclude doesn't constrain its second type parameter on the first, so we felt like it would be strange to do that with Omit.

@DanielRosenwasser
Copy link
Member Author

Note, if you feel that shouldn't be the case, I'd encourage you to open up a separate issue to discuss it.

@seansfkelley seansfkelley mentioned this pull request Apr 9, 2019
5 tasks
@urish
Copy link

urish commented Apr 13, 2019

@DanielRosenwasser the BigTSQuery dataset has last been updated around June 2018. If you find it useful, I can spend some time updating it

@felixfbecker
Copy link
Contributor

Can Omit please be made strict before 3.5 moves to stable? #30825

@RyanCavanaugh
Copy link
Member

@felixfbecker the definition was intentional.

@leoyli
Copy link

leoyli commented May 19, 2019

@DanielRosenwasser

Exclude doesn't constrain its second type parameter on the first, so we felt like it would be strange to do that with Omit.

But how a bout the case of Pick? Pick do constrain the use case.

Pick: pick members from a group to get a sub-group;
Omit: omit members in a group to get a sub-group;

As you can see, these 2 helper types are somehow complimentary. Looks pretty nature and intuitive when we use them together. How come Omit being permissive and Pick isn't???

When we make comparison, compare Omit with Exclude is like compare egg with chicken. Exclude is comparable with Extract not Omit, which should be Pick, IMHO.

@OliverJAsh
Copy link
Contributor

I've ran into a few bugs which would have been caught if K was constrained (like it is for Pick). Any chance of reconsidering this?

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.

Add Omit as a known interface
9 participants