Skip to content

Enable XML doc error#2191

Merged
eerhardt merged 2 commits intomicrosoft:mainfrom
eerhardt:XmlDocsWArning
Feb 13, 2024
Merged

Enable XML doc error#2191
eerhardt merged 2 commits intomicrosoft:mainfrom
eerhardt:XmlDocsWArning

Conversation

@eerhardt
Copy link
Copy Markdown
Member

@eerhardt eerhardt commented Feb 12, 2024

Fail the build when a public member is not documented.

Since there are so many violations, for the current ones that weren't straight forward, I added a TODO for someone more familiar with the API to add the comments.

Microsoft Reviewers: Open in CodeFlow

@ghost ghost added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Feb 12, 2024
namespace Aspire.Hosting.ApplicationModel;

/// <summary>
/// TODO: Doc Comments
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.

Don't know if you did already, but it'd be great if we also log an issue for all of these TODO's that were added. Of course just a single issue for all is sufficient.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I plan on logging it once this change is merged.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Logged #2213.

namespace Aspire.Hosting.Publishing;

public class ManifestPublisher(ILogger<ManifestPublisher> logger,
internal class ManifestPublisher(ILogger<ManifestPublisher> logger,
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.

Same here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think this deserves a one-off breaking change notice. I don't expect users to have used this type in their code. We could roll it in with the other breaking change notices for the appmodel.

cc @mitchdenny

Copy link
Copy Markdown
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

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

Minor comments but looks good. Thanks a bunch for taking care of this @eerhardt!

Fail the build when a public member is not documented.

Since there are so many violations, for the current ones that weren't straight forward, I added a TODO for someone more familiar with the API to add the comments.
@eerhardt eerhardt enabled auto-merge (squash) February 13, 2024 22:45
@eerhardt eerhardt mentioned this pull request Feb 13, 2024
@eerhardt eerhardt merged commit fffda7c into microsoft:main Feb 13, 2024
@eerhardt eerhardt deleted the XmlDocsWArning branch February 14, 2024 03:59
radical pushed a commit to radical/aspire that referenced this pull request Feb 14, 2024
* Enable XML doc error

Fail the build when a public member is not documented.

Since there are so many violations, for the current ones that weren't straight forward, I added a TODO for someone more familiar with the API to add the comments.

* Add XML docs for new APIs
@github-actions github-actions bot locked and limited conversation to collaborators Apr 21, 2024
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.

3 participants