Skip to content

Conversation

sebastienros
Copy link
Member

@sebastienros sebastienros commented Oct 22, 2024

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #6298

Diagnostic documented here dotnet/docs-aspire#1880

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?
    • Yes
      • Link to aspire-docs issue:
    • No
Microsoft Reviewers: Open in CodeFlow

Copy link
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.

We also need to "stablize" the package again.

<!-- This needs to be set to true before importing parent Directory.Build.props -->
<!-- In preview until the public API is validated. -->
<SuppressFinalPackageVersion>true</SuppressFinalPackageVersion>
</PropertyGroup>


using System.Diagnostics.CodeAnalysis;

[assembly: Experimental("ASPIREHOSTINGPYTHON001", UrlFormat = "https://aka.ms/dotnet/aspire/diagnostics#{0}")]
Copy link
Member

Choose a reason for hiding this comment

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

How do we get https://learn.microsoft.com/en-us/dotnet/aspire/diagnostics/overview#ASPIREHOSTINGPYTHON001 to be a valid URL?

cc @IEvangelist

Copy link
Member

Choose a reason for hiding this comment

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

The ASPIREHOSTINGPYTHON001 has to either be a heading, or you can use an inline named HTML element:

<span name="ASPIREHOSTINGPYTHON001"></span>

Then the bookmark will work.

Copy link
Member Author

Choose a reason for hiding this comment

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

@IEvangelist when I create the doc I use a heading, same as the other sections, but the generated id is lower-cased so the current link doesn't work. I will add the this way there is no change to do in aspire, but it's unfortunate. Maybe the doc generator could also inject a name with the actual case of the heading (though that would leak everywhere).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's a dotnet/docfx concern more than anything else. Bookmarks in URLs just look better in lowercase in my opinion, but knowing that they're always in lowercase, why not just have this code ensure that they're lowercase?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, had I known I would have done it before. But I don't want to block release/9.0 insertion on that. Will do it in main once 9.0 has shipped and is merged back.

Copy link
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.

Code change looks good to me. We will need to get the doc site updated for the new diagnostic.

@sebastienros sebastienros merged commit 8090fda into main Oct 23, 2024
9 checks passed
@sebastienros sebastienros deleted the sebros/pythonexperimental branch October 23, 2024 16:31
@sebastienros
Copy link
Member Author

/backport to release/9.0

Copy link
Contributor

Started backporting to release/9.0: https://github.com/dotnet/aspire/actions/runs/11484230088

@joperezr
Copy link
Member

Code change looks good to me. We will need to get the doc site updated for the new diagnostic.

@sebastienros can you please log an issue for this in the aspire-docs repo and paste it above in the PR description form? That way when filling that doc we'll have a link to the original PR.

@eerhardt
Copy link
Member

Code change looks good to me. We will need to get the doc site updated for the new diagnostic.

@sebastienros can you please log an issue for this in the aspire-docs repo and paste it above in the PR description form? That way when filling that doc we'll have a link to the original PR.

This happened with Document experimental attributes for Aspire.Hosting.Python (dotnet/docs-aspire#1880).

@github-actions github-actions bot locked and limited conversation to collaborators Nov 25, 2024
@github-actions github-actions bot added the area-integrations Issues pertaining to Aspire Integrations packages label Mar 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-integrations Issues pertaining to Aspire Integrations packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mark AddPythonApp API as experimental
4 participants