Skip to content
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

Fix triple slash usage of inheritdoc. #6718

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

IEvangelist
Copy link
Member

@IEvangelist IEvangelist commented Nov 18, 2024

Description

Many of the container-based hosting integrations include an internal tags class, that stores the various container registry, image and tags. Since these members are internal the .NET Aspire API docs doesn't expose them in the rendered API docs. Nor are they available to IntelliSense.

For example, see RedisBuilderExtensions.AddRedis Method: Remarks.

To fix this, when we rely on <inheritdoc /> we must specify the path (XPath) to the desired value. Before my change:

image

After my change:

image

This will also fix the .NET Aspire API docs too, the next time we release.

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?
Microsoft Reviewers: Open in CodeFlow

…heir triple slash, but not correctly providing the path to resolve the values from.
@eerhardt
Copy link
Member

Can you fix these 2 as well?

/// <summary>
/// Gets the name of the SKU.
/// </summary>
/// <value>
/// The default value is <inheritdoc cref="DefaultSkuName"/>.
/// </value>
public string SkuName { get; set; } = skuName ?? DefaultSkuName;
/// <summary>
/// Gets the capacity of the SKU.
/// </summary>
/// <value>
/// The default value is <inheritdoc cref="DefaultSkuCapacity"/>.
/// </value>
public int SkuCapacity { get; set; } = skuCapacity ?? DefaultSkuCapacity;

@eerhardt
Copy link
Member

There are 3 Azure ones that appear to be missed:

/// This version of the package defaults to the <inheritdoc cref="StorageEmulatorContainerImageTags.Tag"/> tag of the <inheritdoc cref="StorageEmulatorContainerImageTags.Registry"/>/<inheritdoc cref="StorageEmulatorContainerImageTags.Image"/> container image.

/// This version of the package defaults to the <inheritdoc cref="CosmosDBEmulatorContainerImageTags.Tag"/> tag of the <inheritdoc cref="CosmosDBEmulatorContainerImageTags.Registry"/>/<inheritdoc cref="CosmosDBEmulatorContainerImageTags.Image"/> container image.

/// This version of the package defaults to the <inheritdoc cref="EventHubsEmulatorContainerImageTags.Tag"/> tag of the <inheritdoc cref="EventHubsEmulatorContainerImageTags.Registry"/>/<inheritdoc cref="EventHubsEmulatorContainerImageTags.Image"/> container image.

(Although, that EventHubs one appears misplaced. It should be on the RunAsEmulator method.)

@eerhardt
Copy link
Member

Can you make sure this works as expected? This is a 2-level reference:

/// <summary><inheritdoc cref="Tag"/>-management</summary>
public const string ManagementTag = $"{Tag}-management";

/// This version of the package defaults to the <inheritdoc cref="RabbitMQContainerImageTags.ManagementTag"/> tag of the <inheritdoc cref="RabbitMQContainerImageTags.Image"/> container image.

@sebastienros
Copy link
Member

I recently created a PR that was fixing all these. Didn't we merge release/9.0 back to main? Pretty sure I updated everything to use in both the APIs and the tags.

@sebastienros
Copy link
Member

@sebastienros
Copy link
Member

release/9.0 is fine, but the PR didn't get merged back correctly apparently (conflicts got resolved using main) #6433

@IEvangelist
Copy link
Member Author

There might be an issue with the .NET API docs. I'm investigating that and will report back.

@sebastienros sebastienros mentioned this pull request Nov 20, 2024
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants