Skip to content

Conversation

desjoerd
Copy link
Contributor

@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
Contributor 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
Contributor Author

desjoerd commented Jun 2, 2025

@dotnet-policy-service agree

@desjoerd
Copy link
Contributor 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
Contributor 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
Contributor 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