Convert ResourceUrlAnnotation.DisplayOrder from field to property#13785
Convert ResourceUrlAnnotation.DisplayOrder from field to property#13785
Conversation
Co-authored-by: JamesNK <303201+JamesNK@users.noreply.github.com>
JamesNK
requested review from
DamianEdwards,
captainsafia,
davidfowl and
mitchdenny
|
🚀 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.
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 <175728472+Copilot@users.noreply.github.com>
| /// 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.
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.
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.
If we want to break it I think we should wait for a major version.
There was a problem hiding this comment.
Good point. If 13.2 is the next release then this should wait.
There was a problem hiding this comment.
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.
DamianEdwards
added
the
breaking-change
mitchdenny
dismissed
their stale review
Dismissing own review since consensus is to wait since this is a binary breaking change. That said if we are making other binary breaking changes we should slip this one in at the same time.
JamesNK
added
the
NO-MERGE
|
Closing as we aren't doing this now. |
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.