-
Notifications
You must be signed in to change notification settings - Fork 769
Convert ResourceUrlAnnotation.DisplayOrder from field to property #13785
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: JamesNK <[email protected]>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13785Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13785" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR converts ResourceUrlAnnotation.DisplayOrder from a public field to a property with get/set accessors, aligning with .NET design guidelines and ensuring consistency with other members of the class. While the PR description mentions adding a CP0008 suppression to CompatibilitySuppressions.xml, this file is not included in the provided diff.
Key Changes
- Changed
DisplayOrderfrom a field to a property with auto-implemented getter and setter - Updated API baseline file to reflect the new property signature
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Aspire.Hosting/ApplicationModel/ResourceUrlAnnotation.cs | Converted DisplayOrder field to property with get/set accessors |
| src/Aspire.Hosting/api/Aspire.Hosting.cs | Updated API baseline to reflect property signature change |
Co-authored-by: Copilot <[email protected]>
| /// The display order of the URL. Higher values mean sort higher in the list. | ||
| /// </summary> | ||
| public int? DisplayOrder; | ||
| public int? DisplayOrder { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a binary-breaking change I believe. It is currently being used from the CommunityToolkit too: https://github.com/CommunityToolkit/Aspire/blob/5f8321271f9c3630d5758c44dae3d014cf1e7237/src/CommunityToolkit.Aspire.Hosting.McpInspector/McpInspectorResourceBuilderExtensions.cs#L96
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it would be a breaking change. Either we fix it now or never change it.
Do you have suggestions to mitigate? We could put an issue in the CT repo to let them know and document that MCP inspector requires a certain version of Aspire
cc @aaronpowell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to break it I think we should wait for a major version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. If 13.2 is the next release then this should wait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have time before this releases, we can help smooth out breaking change by removing usage in community toolkit - CommunityToolkit/Aspire#1088. Then re-add it once CT upgrades to vnext major.
Description
ResourceUrlAnnotation.DisplayOrderwas a public field. Converted to a property with get/set accessors for consistency with other public API members and to follow .NET design guidelines.Changes
public int? DisplayOrder;topublic int? DisplayOrder { get; set; }The change is backward compatible for all existing usage patterns (read/write).
Checklist
Original prompt
💡 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.