Skip to content

Improve XML documentation comments#7728

Merged
mitchdenny merged 10 commits intomainfrom
mitchdenny/improve-waitbehavior-docs
Feb 21, 2025
Merged

Improve XML documentation comments#7728
mitchdenny merged 10 commits intomainfrom
mitchdenny/improve-waitbehavior-docs

Conversation

@mitchdenny
Copy link
Copy Markdown
Member

Enhance clarity and detail in XML documentation comments for resource health checks and wait behaviors.

Copilot AI review requested due to automatic review settings February 21, 2025 02:57
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs:235

  • There is an inconsistency in the terminology used in documentation. In this method the term 'terminal state' is used, whereas in the related overload documentation another file refers to 'unhealthy terminal state'. Consider standardizing the phrasing for clarity.
/// If the resource enters a terminal state such as <see cref="KnownResourceStates.FailedToStart"/> then

src/Aspire.Hosting/ResourceBuilderExtensions.cs:737

  • The XML comment here uses 'unhealthy terminal state' while similar documentation in ResourceNotificationService.cs uses 'terminal state'. Aligning the language across methods would improve consistency.
/// will throw a <see cref="DistributedApplicationException"/> if the resource enters an unhealthy terminal state.

…e.cs

Co-authored-by: Dan Moseley <danmose@microsoft.com>
Copy link
Copy Markdown
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

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

terminal -> unavailable

I think this is better because the enum option uses the word "Unavailable" and that's more accurate because logic includes some non terminal states.

@danmoseley danmoseley added this to the 9.1 milestone Feb 21, 2025
mitchdenny and others added 4 commits February 21, 2025 17:42
Co-authored-by: James Newton-King <james@newtonking.com>
…e.cs

Co-authored-by: James Newton-King <james@newtonking.com>
…e.cs

Co-authored-by: James Newton-King <james@newtonking.com>
…e.cs

Co-authored-by: James Newton-King <james@newtonking.com>
Comment on lines +264 to +265
/// then the method will continue to wait until the resource reaches a <see cref="KnownResourceStates.Running"/> state. This is the default
/// behavior with the <see cref="WaitForResourceHealthyAsync(string, CancellationToken)"/> overload.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
/// then the method will continue to wait until the resource reaches a <see cref="KnownResourceStates.Running"/> state. This is the default
/// behavior with the <see cref="WaitForResourceHealthyAsync(string, CancellationToken)"/> overload.
/// then the method will continue to wait until the resource reaches a <see cref="KnownResourceStates.Running"/> state.

I would remove this sentence since:

  1. It is about a different method than what we are documenting here.
  2. It isn't accurate since the ResourceNotificationServiceOptions.DefaultWaitBehavior can (and will) change the behavior of that method.

mitchdenny and others added 4 commits February 22, 2025 09:26
…e.cs

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
…e.cs

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
…e.cs

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Copy link
Copy Markdown
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@mitchdenny mitchdenny merged commit 33ddfb5 into main Feb 21, 2025
@mitchdenny mitchdenny deleted the mitchdenny/improve-waitbehavior-docs branch February 21, 2025 22:46
@mitchdenny
Copy link
Copy Markdown
Member Author

/backport to release/9.1

@github-actions
Copy link
Copy Markdown
Contributor

Started backporting to release/9.1: https://github.com/dotnet/aspire/actions/runs/13466532044

@github-actions github-actions bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Mar 10, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Apr 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants