Skip to content

Fix OpenAPI Culture formatting for RangeAttribute and Unit Tests #62212

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

Closed
wants to merge 1 commit into from

Conversation

desjoerd
Copy link

@desjoerd desjoerd commented Jun 2, 2025

Fix OpenAPI Culture formatting for RangeAttribute and Unit Tests

This is to fix unit tests and formatting issues when having a culture with , as decimal seperator.

Description

This is to fix unit tests and formatting issues when having a culture with , as decimal seperator. It fixes the range attribute by formatting it using the target locale. And the xml comment tests by using the FormattingStreamWriter as mentioned in:
microsoft/OpenAPI.NET#657 (comment).

Partially Fixes progress for #61965

…d Update tests to write in the InvariantCulture
@desjoerd desjoerd requested review from captainsafia and a team as code owners June 2, 2025 12:38
@github-actions github-actions bot added the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label Jun 2, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 2, 2025
@desjoerd
Copy link
Author

desjoerd commented Jun 2, 2025

@captainsafia I've created a PR which makes the failing unit tests green and should resolve a few formatting issues. (So I can make the fix for #61965)

I am partially doubting whether this PR is a good thing, yes it fixes the issue, and it should fix it for unit tests, but I think it hides an underlying issue (in Microsoft.OpenAPI) that it writes invalid json when the culture has a , and you don't use the FormattingWriter with CultureInfo.Invariant.

@martincostello martincostello added feature-openapi area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically labels Jun 2, 2025
@desjoerd
Copy link
Author

desjoerd commented Jun 2, 2025

@dotnet-policy-service agree

@desjoerd
Copy link
Author

desjoerd commented Jun 3, 2025

@martincostello let me know if I need to change something or update my branch when the changes made in #62193 are merged.

@martincostello
Copy link
Member

@desjoerd I updated #62193 to improve the tests, and now the test changes I made have repro'd the bug you're trying to fix here.

Would you mind if I pull your fix commit into my branch? Otherwise it's a bit chicken and egg as my branch can't be merged as another bug causes the tests that detect the bug I'm trying to fix to still fail. 😄

@desjoerd
Copy link
Author

desjoerd commented Jun 4, 2025

Feel free to do that :).

(Will I still get the first contribution credit 😜)

@martincostello
Copy link
Member

Will I still get the first contribution credit

Of course!

@desjoerd
Copy link
Author

desjoerd commented Jun 4, 2025

When you've done it and give the 👍 I will close this PR.

@martincostello
Copy link
Member

It's now included in #62193.

@desjoerd desjoerd closed this Jun 4, 2025
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview6 milestone Jun 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc community-contribution Indicates that the PR has been added by a community member feature-openapi
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants