Skip to content

Remove KeyComparer property from OpenApiWriterSettings #2375

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

Merged
merged 5 commits into from
Jun 4, 2025
Merged

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Jun 4, 2025

Following discussions in #2363, this PR removes the KeyComparer property from OpenApiWriterSettings class as ordered collections are sufficient for the library's needs.

Changes Made

  • Removed KeyComparer property from OpenApiWriterSettings.cs
  • Updated writer logic in OpenApiWriterExtensions.cs to remove custom sorting:
    • Removed KeyComparer usage from WriteCollectionInternal method
    • Removed KeyComparer usage from WriteMapInternal method
  • Cleaned up tests in CollectionExtensionsTests.cs:
    • Removed SortOpenApiDocumentUsingCustomComparerSucceeds test
    • Removed SortHashSetsWorks test
    • Removed SortTagsByNameUsingComparerWorks test and associated OpenApiTagNameComparer class
  • Removed unused extension methods from CollectionExtensions.cs:
    • Removed Sort<TKey, TValue> method for dictionaries
    • Removed Sort<T> method for enumerables
  • Updated public API surface in PublicApi.approved.txt to reflect removed property

Impact

The removal of custom comparer functionality aligns with the decision to rely on ordered collections instead. Collections and maps will now be written in their natural order rather than using custom sorting logic.

Testing

  • All existing functionality continues to work as expected
  • Build passes successfully for all target frameworks
  • No breaking changes to core serialization behavior

Fixes #2374.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • http://168.63.129.16:80/machine/
    • Triggering command: /usr/bin/python3 -u bin/WALinuxAgent-2.13.1.1-py3.9.egg -collect-logs (http block)
  • http://www.microsoft.com:80/pkiops/crl/Microsoft%20Identity%20Verification%20Root%20Certificate%20Authority%202020.crl
    • Triggering command: dotnet build (http block)
  • http://www.microsoft.com:80/pkiops/crl/Microsoft%20Public%20RSA%20Timestamping%20CA%202020.crl
    • Triggering command: dotnet build (http block)

If you need me to access, download, or install something from one of these locations, you can either:


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@Copilot Copilot AI changed the title [WIP] Remove the comparer property from the writer settings Remove KeyComparer property from OpenApiWriterSettings Jun 4, 2025
@Copilot Copilot AI requested a review from baywet June 4, 2025 18:57
Copilot finished work on behalf of baywet June 4, 2025 18:58
Copy link
Contributor Author

Copilot AI commented Jun 4, 2025

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • http://168.63.129.16:80/machine/
    • Triggering command: /usr/bin/python3 -u bin/WALinuxAgent-2.13.1.1-py3.9.egg -collect-logs (http block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot finished work on behalf of baywet June 4, 2025 19:10
@baywet baywet marked this pull request as ready for review June 4, 2025 19:13
@baywet baywet requested a review from a team as a code owner June 4, 2025 19:13
@baywet baywet enabled auto-merge (squash) June 4, 2025 19:13
Copy link

sonarqubecloud bot commented Jun 4, 2025

@adrian05-ms adrian05-ms self-requested a review June 4, 2025 19:25
@peombwa
Copy link
Contributor

peombwa commented Jun 4, 2025

@baywet, shouldn't the PR title be renamed to match conventional commit. i.e. fix: XYZ?

@baywet
Copy link
Member

baywet commented Jun 4, 2025

@baywet, shouldn't the PR title be renamed to match conventional commit. i.e. fix: XYZ?

@peombwa why would you expect the PR title to match the conventional commit name?

@peombwa
Copy link
Contributor

peombwa commented Jun 4, 2025

@baywet, shouldn't the PR title be renamed to match conventional commit. i.e. fix: XYZ?

@peombwa why would you expect the PR title to match the conventional commit name?

@baywet, this is based on the naming convention used for closed PRs in this repo - https://github.com/microsoft/OpenAPI.NET/pulls?q=is%3Apr+is%3Aclosed. However, based on the releases, it appears that release notes are generated from commit messages rather than PR titles.

Copy link
Contributor

@peombwa peombwa left a comment

Choose a reason for hiding this comment

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

LGTM

@baywet baywet merged commit 7e6e7f3 into main Jun 4, 2025
17 checks passed
@baywet baywet deleted the copilot/fix-2374 branch June 4, 2025 21:12
@baywet
Copy link
Member

baywet commented Jun 4, 2025

Yes release notes are using the commits messages.
The GitHub UI adds the PR title in the merge commit message.
The GitHub CLI uses the commit message as PR title if it's a single commit.
This is why things can appear multiple places.

@bkoelman
Copy link
Contributor

bkoelman commented Jun 5, 2025

Thanks!

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.

Remove the comparer property from the writer settings
5 participants