-
Notifications
You must be signed in to change notification settings - Fork 117
Allow overrides for disallowed types (e.g. 64 bit integers) #140
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
base: main
Are you sure you want to change the base?
Allow overrides for disallowed types (e.g. 64 bit integers) #140
Conversation
I swapped my install to this this fork. It is working. |
@Lucretiel @inquisitivepenguin Sorry to bother, but could I get a review on this? |
This closes #112 for sure. A very beautiful solution I might add! |
I'd like to see this included as well. You may also want to add u128 and i128 to the supported types. |
@anish-1p sorry to bother you -- would you mind taking a look at this PR and reviewing or merging? Thank you! |
@tomjw64 Thanks for putting this together - I've just followed a trail of breadcrumbs here and you've helped me out a lot! |
@charlespierce can you review this? This is a blocker for many of us working with 64 bits ints. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this! The change seems perfectly reasonable for 64 bit types (i64
& u64
). My only concerns are around the pointer-width types usize
and isize
for two reasons:
- These types aren't always the same size depending on the target platform, so translating them into types for other languages could potentially be incorrect.
- It seems that the individual language implementations are inconsistent with how they handle
usize
andisize
—Scala & Kotlin treat them as 32-bit integers, Swift & Go treat them as platform-specific size.
The inconsistency doesn't matter currently, since the parser error means they aren't ever instantiated in the first place.
Would it be unusable to make this change for i64
and u64
but leave isize
and usize
as unsupported types?
@charlespierce Thanks for the review! Could these both be resolved by also requiring type overrides for isize and usize (but not i64 and u64) for the other languages? That should be a requirement that could get folks unblocked but also that could be relaxed in the future while maintaining backwards-compatibility. |
Yeah, updating the other languages to also require an override for |
@charlespierce Updated to also require overrides for all languages for pointer sized types and updated docs. LMK if you would like any other changes! |
Here's an alternative approach to support 64 bit integer types than the one taken in #112.
Rather than blessing one approach, with #119 being merged, it seems like it's possible to accept this oddity of TypeScript/JavaScript and force users of Typeshare (or rather, only those using it to output TypeScript) to make a decision appropriate for them and their use-case.
More or less resolves #24