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

Allow number codecs to specify that their values must be integers #36

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dmoverton
Copy link
Contributor

@dmoverton dmoverton commented Apr 13, 2023

Add a field to the NumberCodec that specifies whether or not the value is required to be an integer. The main reason for doing this is to ensure that more precise schemas are generated for OpenAPI, Swagger and JSON Schema.

OpenAPI, Swagger and JSON Schema all support "type": "integer" as a type specifier, but because NumberCodec was not previously able to differentiate between integral and non-integral types it was generating schemas with "type": "number" for integral types.

@NorfairKing
Copy link
Owner

Just so we don't get in trouble with this again: are there any other numbers than integers and not integers?

CI fails as well, so that definitely needs to be fixed first, but I like the idea of this PR.

@dmoverton
Copy link
Contributor Author

Just so we don't get in trouble with this again: are there any other numbers than integers and not integers?

Looking at the OpenAPI 3 spec, there is an optional format property which for number can be float or double, and for integer can be int32 or int64. We want to use the OpenAPI 3 spec to generate Rust types, so I suspect this extra information would be useful. I'll have a go at that.

The ToSchema instances in the Haskell openapi3 library actually generate the format property for the Haskell types Float, Double, Int32 and Int64. Unfortunately, it's not easy for Autodocodec to use those instances.

CI fails as well, so that definitely needs to be fixed first, but I like the idea of this PR.

It's failing with

Error: Unable to locate executable file: cachix

I'm not sure why my changes would be causing that error. Are you able to investigate that?

@NorfairKing
Copy link
Owner

Looking at the OpenAPI 3 spec, there is an optional format property which for number can be float or double, and for integer can be int32 or int64. We want to use the OpenAPI 3 spec to generate Rust types, so I suspect this extra information would be useful. I'll have a go at that.

In that case we could actually encode this information in the number codec.

Are you able to investigate that?

Yes we probably just need to bump the github actions' versions.
As long as nix flake check passes, that's fine by me.

@NorfairKing
Copy link
Owner

Ping @dmoverton

@dmoverton
Copy link
Contributor Author

Ping @dmoverton

Hi. I did do some work on using the "format" string for number, but I wasn't completely happy with it and then I got distracted by something else and never got back to it. I'll see if I can get something to you in the next day or so.

@NorfairKing
Copy link
Owner

@dmoverton Any news?

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