Skip to content

Conversation

Copy link

Copilot AI commented Dec 15, 2025

Pull Request

Description

Addresses review feedback on PR microsoft#2645 by replacing .NET 5+ specific Enum.IsDefined with clear iteration logic and updating test assertions to use structural JSON comparison instead of string comparison.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Other (please describe):

Related Issue(s)

Addresses review comments on https://github.com/microsoft/OpenAPI.NET/pull/2645/files

Changes Made

  • Replaced System.Enum.IsDefined(commonType) with clear iteration logic that explicitly checks if all schemas have the same type
    • Avoids .NET 5+ dependency and trimming issues
    • Uses & ~JsonSchemaType.Null bitwise operator to remove Null flag from type comparison, consistent with existing patterns in the class
    • More readable and maintainable code
  • Updated 7 test methods to use JsonNode.DeepEquals instead of string comparison with MakeLineBreaksEnvironmentNeutral()
    • More robust structural comparison
    • Platform-agnostic line ending handling

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed
  • All existing tests pass

Checklist

  • My code follows the code style of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Versions applicability

  • My change applies to the version 1.X of the library, if so PR link:
  • My change applies to the version 2.X of the library, if so PR link:
  • My change applies to the version 3.X of the library, if so PR link:
  • I have evaluated the applicability of my change against the other versions above.

See the contributing guidelines for more information about how patches are applied across multiple versions.

Additional Notes

All 927 tests pass. Changes maintain backward compatibility while improving cross-framework support and code readability. The implementation uses the established bitwise pattern from the class for handling null flags.

Original prompt

Can you fix the comments on the PR https://github.com/microsoft/OpenAPI.NET/pull/2645/files


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

…and use JsonNode.DeepEquals in tests

Co-authored-by: desjoerd <[email protected]>
Copilot AI changed the title [WIP] Fix comments on PR 2645 Address PR #2645 review comments: replace Enum.IsDefined and use JsonNode.DeepEquals Dec 15, 2025
Copilot AI requested a review from desjoerd December 15, 2025 19:50
Copilot AI changed the title Address PR #2645 review comments: replace Enum.IsDefined and use JsonNode.DeepEquals Address PR #2645 review comments: improve type checking logic and use JsonNode.DeepEquals Dec 15, 2025
Copilot AI requested a review from desjoerd December 15, 2025 23:00
Copy link
Owner

@desjoerd desjoerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment on on the first type logic.

You should still remove null from the equation. You can still do that with a bit wise operator as that is done at more places in this class.

Copy link
Author

Copilot AI commented Dec 15, 2025

See my comment on on the first type logic.

You should still remove null from the equation. You can still do that with a bit wise operator as that is done at more places in this class.

Updated to use & ~JsonSchemaType.Null bitwise operator to remove the Null flag from type comparison, consistent with other usage in the class. Commit: 3f2402f

@desjoerd desjoerd marked this pull request as ready for review December 15, 2025 23:56
@desjoerd desjoerd merged commit b8ad06f into fix/support-v2/gh-2629 Dec 15, 2025
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.

2 participants