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

[API Review] Decimal vs Double for Max/Min in OpenAPISchema type #1967

Open
RachitMalik12 opened this issue Nov 26, 2024 · 5 comments
Open
Assignees
Labels
priority:p3 Nice to have. Customer impact is very minimal status:waiting-for-internal-feedback Needs internal stakeholder(s) input to resolve
Milestone

Comments

@RachitMalik12
Copy link

In the API review, the question of why the (Exclusive)Max/(Exclusive)Min properties was chosen to be of type Decimal vs Double.
@darrelmiller has a contact to follow up to get an answer and this issue is used as a tracker for this.

@RachitMalik12 RachitMalik12 added this to the Backlog milestone Nov 26, 2024
@RachitMalik12 RachitMalik12 added the priority:p3 Nice to have. Customer impact is very minimal label Nov 27, 2024
@baywet baywet modified the milestones: Backlog, v2-Preview12 Feb 27, 2025
@baywet baywet marked this as a duplicate of #1122 Feb 27, 2025
@RachitMalik12 RachitMalik12 added the status:waiting-for-internal-feedback Needs internal stakeholder(s) input to resolve label Feb 27, 2025
@RachitMalik12
Copy link
Author

@darrelmiller you had a contact here to provide more context, can you follow up?

@RachitMalik12
Copy link
Author

Related PR: #1594

@RachitMalik12
Copy link
Author

Proposal: Change to string and the format should be used to interpret whether decimal or double.

@darrelmiller
Copy link
Member

darrelmiller commented Mar 12, 2025

On deserialize we read as JsonValue.GetValue() that we convert to a string to store in the maximum/minimum property in OpenApiSchema. On writing we take the string and use WriteRaw so that no quotes are added.

/cc @mikekistler for thoughts.

/cc @MaggieKimani1 for testing to see if GetValue works for JSON numbers.

@mikekistler
Copy link
Member

This seems like the most flexible and complete solution to me. @captainsafia do you have any concerns with this approach?

@MaggieKimani1 MaggieKimani1 self-assigned this Mar 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:p3 Nice to have. Customer impact is very minimal status:waiting-for-internal-feedback Needs internal stakeholder(s) input to resolve
Projects
None yet
Development

No branches or pull requests

5 participants