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.

Sign up for GitHub

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

Jump to bottom

Rename projectDirectory to appDirectory in AddPythonApp #7169

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Rename projectDirectory to appDirectory in AddPythonApp #7169

wants to merge 3 commits into from

Conversation

matthebrown
Copy link
Contributor

@matthebrown matthebrown commented Jan 20, 2025 •
edited
Loading

Description

Renames the projectDirectory parameter to appDirectory in AddPythonApp.

Fixes #6242

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?

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jan 20, 2025
@matthebrown
Copy link
Contributor Author

@dotnet-policy-service agree

@dotnet-policy-service agree

DamianEdwards
DamianEdwards requested changes Jan 30, 2025
View reviewed changes
Copy link
Member

@DamianEdwards DamianEdwards left a comment

Choose a reason for hiding this comment

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

Still a few comments to update

src/Aspire.Hosting.Python/PythonAppResourceBuilderExtensions.cs
@@ -17,7 +17,7 @@ public static class PythonAppResourceBuilderExtensions
/// </summary>
/// <param name="builder">The <see cref="IDistributedApplicationBuilder"/> to add the resource to.</param>
/// <param name="name">The name of the resource.</param>
/// <param name="projectDirectory">The path to the directory containing the python app files.</param>
/// <param name="appDirectory">The path to the directory containing the python app files.</param>
/// <param name="scriptPath">The path to the script relative to the project directory to run.</param>
Copy link
Member

Choose a reason for hiding this comment

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

This still says project directory but I think it's referring to the python app directory?

src/Aspire.Hosting.Python/PythonAppResourceBuilderExtensions.cs
@@ -53,25 +53,25 @@ public static class PythonAppResourceBuilderExtensions
/// var builder = DistributedApplication.CreateBuilder(args);
///
/// builder.AddPythonApp("python-project", "PythonProject", "main.py");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// builder.AddPythonApp("python-project", "PythonProject", "main.py");
/// builder.AddPythonApp("python-project", "PythonApp", "main.py");

src/Aspire.Hosting.Python/PythonAppResourceBuilderExtensions.cs
@@ -102,44 +102,44 @@ public static IResourceBuilder<PythonAppResource> AddPythonApp(
/// var builder = DistributedApplication.CreateBuilder(args);
///
/// builder.AddPythonApp("python-project", "PythonProject", "main.py");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// builder.AddPythonApp("python-project", "PythonProject", "main.py");
/// builder.AddPythonApp("python-app", "PythonApp", "main.py");

@dotnet-policy-service dotnet-policy-service bot added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DamianEdwards DamianEdwards DamianEdwards requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
community-contribution Indicates that the PR has been added by a community member needs-author-action An issue or pull request that requires more info or actions from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rename projectDirectory to appDirectory in AddPythonApp
2 participants
@matthebrown @DamianEdwards