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 "format" field for uint32 and uint64 #215

Merged
merged 2 commits into from
May 15, 2020

Conversation

paulyoung
Copy link
Contributor

@paulyoung paulyoung commented May 14, 2020

We noticed that for int32 and int64 fields we were able to check the format field and decode an int64 value to a BigInt in PureScript (ultimately JavaScript) but for uint32 and uint64 that field wasn't present.

This pull request adds that field along with a test to verify that it works as expected.

Copy link
Collaborator

@fisx fisx left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

I'm wondering about the shorter words though. Do they behave differently in purescript / js? Should we?

@paulyoung
Copy link
Contributor Author

paulyoung commented May 14, 2020

To my knowledge, the Swagger 2 spec only provides int32 and int64 for format. I’m not sure if the former should be used for smaller sizes.

On the plus side it might allow us to use PureScript’s Int type instead of Number. On the down side, some consumers might be concerned about using a larger sized integer than is necessary.

@paulyoung
Copy link
Contributor Author

I'm beginning to think we should perhaps have just looked at the minimum and maximum fields all along...

@paulyoung
Copy link
Contributor Author

Apparently the format is necessary because the wire type may not actually be a number.

@paulyoung
Copy link
Contributor Author

More specifically, the min and max is not enough information to determine if it’s an integer that’s a numeric type, or an integer that’s actually a string.

@paulyoung
Copy link
Contributor Author

Using int32 for uint32 (for example) does mean we'd need to check the minimum field in order to determine whether to use a PureScript Int or Number (respectively) since Int doesn't cover the full range of unsigned integers.

@fisx fisx merged commit 023e2ac into GetShopTV:master May 15, 2020
@fisx
Copy link
Collaborator

fisx commented May 15, 2020

I can't see any downsides in what you're providing, nor any obvious improvements, so, merge! But feel free to open another PR if you can think of anything else.

Thanks again!

@paulyoung paulyoung deleted the paulyoung/uints branch May 16, 2020 00:06
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

2 participants