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 support for bigint #366

Merged
merged 3 commits into from
Nov 19, 2024
Merged

Add support for bigint #366

merged 3 commits into from
Nov 19, 2024

Conversation

mdoi2
Copy link
Contributor

@mdoi2 mdoi2 commented Nov 17, 2024

I have implemented support for bigint type 😄
The schema is output as integer(int64).

If I am missing any considerations, please point them out🙇🏻‍♂️

@samchungy
Copy link
Owner

Could you update the readme otherwise lgtm

@mdoi2
Copy link
Contributor Author

mdoi2 commented Nov 17, 2024

Thanks! I had overlooked that.
And I realized that each validation process should be supported like ZodNumber.
I will try to implement it, so please wait.

@mdoi2
Copy link
Contributor Author

mdoi2 commented Nov 17, 2024

Oh... I was wondering why only BIgInt is not supported, maybe because metadevpro/openapi3-ts does not support bigint and you dare not support it?

@samchungy
Copy link
Owner

Thanks! I had overlooked that. And I realized that each validation process should be supported like ZodNumber. I will try to implement it, so please wait.

I'm not entirely sure what you mean by that to be honest. The contents of your PR seem fine to me

@mdoi2
Copy link
Contributor Author

mdoi2 commented Nov 17, 2024

Thanks for the reply, samchungy.

maximum/minimum/multipleOf in SchemaObject is number type, so bigint cannot be set.
I have come up with the following 2 patterns to deal with this issue without changing the type.

  • Exclude maximum/minimum/multipleOf from support and explain it in the README.
  • Supports maximum/minimum/multipleOf only in the range of number. if the number exceeds the range, it will be excluded from the output and display warning using console.warn.

I think the latter will suffice for most of my use cases.
Please give me your opinion on this.

@samchungy
Copy link
Owner

I think if you're using bigint you're not going to really need min/max, not to mention like you said JSON Schema wouldn't be able to display it anyway

@mdoi2
Copy link
Contributor Author

mdoi2 commented Nov 19, 2024

Thanks for your advice🙇🏻‍♂️ certainly agree with your thoughts.
If you can only use min/max within the range of number, you should have used number from the beginning.
And, fortunately, the range of number is wider than int32, so the possibility of needing bigint is itself low.

I have appended it to README.md.
Could you please check if everything is OK?

I did not include a note on the min/max matter, but will write one if necessary.

README.md Outdated Show resolved Hide resolved
Copy link
Owner

@samchungy samchungy left a comment

Choose a reason for hiding this comment

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

Thanks!

@samchungy samchungy merged commit 60cf31c into samchungy:master Nov 19, 2024
1 check passed
@samchungy samchungy added the enhancement New feature or request label Nov 25, 2024
@github-actions github-actions bot mentioned this pull request Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants