Skip to content

Fixes for tests #2

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

Merged
merged 8 commits into from
Mar 12, 2025
Merged

Fixes for tests #2

merged 8 commits into from
Mar 12, 2025

Conversation

james-pre
Copy link

@james-pre james-pre commented Mar 12, 2025

Hi there @skeate, I fixed a few things and now tests pass:

  • Fixed the keys in intrinsicTypeKinds not matching the types in es5.lib.d.ts (intrinsicTypeKinds used Sub, Mul, and Div for keys, but lib.d.ts used Subtract, Multiply, and Divide)
  • Fixed getIntrinsicMappingType not working correctly on Integer<any>, Integer<number>, Integer<bigint>
  • Renamed Integer to Floor, since that is much less ambiguous
  • Added Ceil and Round types
  • Extracted the Calculation conditional in getIntrinsicMappingType
  • Reduced code duplication in getIntrinsicMappingType/2 and applyNumberMapping/2
  • Fixed reference baselines (Some syntax was incorrect)
  • Fixed a name collision in recursiveConditionalCrash4 (Renamed a type from Add to AddTuple)
  • Accepted some other updated baselines

Please take a look at this, I look forward to getting it merged into Typescript!

Fixed keys in `intrinsicTypeKinds` not matching type names in es5.lib.d.ts
Fixed `getIntrinsicMappingType` behaving incorrectly
Fixed incorrect baseline reference for `recursiveConditionalTypes`
Fixed incorrect baseline reference for `intrinsicTypes`
@kungfooman
Copy link

Thanks so much for this awesome PR! I'm impressed by the work you've done and hope it will merge well with upstream too again ✅ 🚀

Are you still working on it or do you want it merged already?

@skeate
Copy link
Owner

skeate commented Mar 12, 2025

Thank you for the PR! I'll be honest, I don't have a lot of hope that the Typescript team will merge this feature; I did it more as a proof-of-concept. I figure even if they did want to add this functionality, they'd probably implement it in their own way. Still, nice to have a working example.

@skeate skeate merged commit dc82234 into skeate:main Mar 12, 2025
23 of 25 checks passed
@james-pre
Copy link
Author

james-pre commented Mar 12, 2025

Are you still working on it or do you want it merged already?

It was ready to be merged

I don't have a lot of hope that the Typescript team will merge this feature

There is clearly demand for it. We've done the hard work, it shouldn't be too difficult for them to review and merge. Hopefully they can take a look at it soon!

@kungfooman
Copy link

There is clearly demand and the fact we come together to keep it updated represents that - but yea, I see both sides 😅

@skeate Just for completeness, maybe you could update the first PR post from Integer to Floor to not confuse/irritate a potential reviewer?

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.

3 participants