Add XML Docs for WithArgs() #1308
Conversation
The wording in the XML docs is based on my cursory review of [issue 177](dotnet#177) and [PR 1121](dotnet#1121). I should add that I only started reviewing Aspire a few hours ago, so apologies if the XML docs aren't accurate - I just figured the best way to get started contributing is to jump right in (CC: @danmoseley). #Fixes dotnet#1294
dotnet-issue-labeler
bot
added
the
area-codeflow
| @@ -81,6 +81,13 @@ public static IResourceBuilder<ContainerResource> AddContainer(this IDistributed | |||
| return builder.WithAnnotation(annotation); | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// Adds the arguments to be passed to a container resource when the container is started. | |||
There was a problem hiding this comment.
This extension method isn't currently constrained to ContainerResource (see line 91 below) so I'm not sure what it does when used with other resource types. We'll likely need to check that and update the doc comments appropriately.
@eerhardt @davidfowl
There was a problem hiding this comment.
I had a look through the implementation. It is definitely container specific.
There was a problem hiding this comment.
It's not specific right now. It also works for executables.
There was a problem hiding this comment.
@mitchdenny @davidfowl so, which is it? Constraining this method to where T : ContainerResource seems wrong if this indeed works for executables too.
There was a problem hiding this comment.
It's both executables and containers.
There was a problem hiding this comment.
OK so for now we should just ensure the doc comments here reflect what it does in both cases.
There was a problem hiding this comment.
Actually I don't think that WithArgs is fully baked. I am wondering if it was actually intended to be private. @BrennanConroy even though it is in ContainerResourceExtensions I can't see where it ends up being used for container initialization.
There was a problem hiding this comment.
aspire/src/Aspire.Hosting/Dcp/ApplicationExecutor.cs
Lines 572 to 579 in 9103442
There was a problem hiding this comment.
OK find references in VS might be having an issue ;)
There was a problem hiding this comment.
Oooh. Its differentiating between the constructor and the type when I find all references :P
DamianEdwards
removed
the
area-codeflow
joperezr
added
the
community-contribution
|
@mitchdenny can you take this and backport it once it's in? |
|
/backport to release/8.0-preview2 |
|
Started backporting to release/8.0-preview2: https://github.com/dotnet/aspire/actions/runs/7180898900 |
The wording in the XML docs is based on my cursory review of issue 177 and PR 1121.
I should add that I only started reviewing Aspire a few hours ago, so apologies if the XML docs aren't accurate - I just figured the best way to get started contributing is to jump right in (CC: @danmoseley, @DamianEdwards).
#Fixes #1294