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

inf default value cannot be encoded in many languages #93

Closed
andreaTP opened this issue Oct 3, 2023 · 4 comments
Closed

inf default value cannot be encoded in many languages #93

andreaTP opened this issue Oct 3, 2023 · 4 comments
Labels
bug Something isn't working

Comments

@andreaTP
Copy link

andreaTP commented Oct 3, 2023

Hi there 👋 and thanks a lot for sharing this spec as it immensely helps!

I'm working on Kiota and while generating clients for your OpenAPI specification I faced a few issues and reported them, but, in this case, I'm convinced that the specification that can be improved.
Here you are defining the default value of an integer with the string "inf" and here I reported upstream the bug found.

Still, there is room for improvement:

  • inf in languages where the integer type is translated(according to the spec) as a Long/Int64 an infinite value is not easily representable
  • using inf as a valid value is only possible in languages that are using Double, BigInt or similar encodings for all of the numeric values
  • inf is effectively a string here and its meaning can vary across different target languages (I'm failing to find a spec that defines this specific string as a valid integer value)

I think that a sensible default would be the value of 32768 since, according to the docs, is currently the maximum number of tokens for the biggest available model.
I'm happy to open a PR if this is the desired resolution of the issue 🙂

@andreaTP
Copy link
Author

andreaTP commented Oct 3, 2023

Just figured out that, depending on the resolution of #28 another alternative is to simply remove the default field.

@logankilpatrick
Copy link
Contributor

Agreed, will remove the default field in an upcoming PR, thank you for flagging!

@logankilpatrick logankilpatrick added the bug Something isn't working label Nov 27, 2023
@andreaTP
Copy link
Author

That's fine as well!
Thanks for getting back.

@andreaTP
Copy link
Author

andreaTP commented Apr 1, 2024

This has been solved on current main:

default: 16

@andreaTP andreaTP closed this as completed Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants
@andreaTP @logankilpatrick and others