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

add --create-manifest-if-needed flag #31231

Merged
merged 16 commits into from
Apr 10, 2023

Conversation

JL03-Yue
Copy link
Member

#15254

Add --create-manifest-if-needed flag
Create tool manifest automatically when installing a new local tool if the flag is on and no manifest exists.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

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

This looks good. The main thing still needed is to add test coverage.

src/Cli/dotnet/ToolManifest/LocalizableStrings.resx Outdated Show resolved Hide resolved
src/Cli/dotnet/ToolManifest/ToolManifestFinder.cs Outdated Show resolved Hide resolved
src/Cli/dotnet/ToolManifest/ToolManifestFinder.cs Outdated Show resolved Hide resolved
src/Cli/Microsoft.DotNet.Cli.Utils/Constants.cs Outdated Show resolved Hide resolved
src/Cli/dotnet/ToolManifest/ToolManifestFinder.cs Outdated Show resolved Hide resolved
src/Cli/dotnet/ToolManifest/ToolManifestFinder.cs Outdated Show resolved Hide resolved
@JL03-Yue JL03-Yue closed this Mar 20, 2023
@JL03-Yue JL03-Yue requested a review from baronfel March 20, 2023 17:38
@JL03-Yue JL03-Yue reopened this Mar 20, 2023
@baronfel baronfel removed their request for review March 20, 2023 17:50
@dsplaisted
Copy link
Member

Per a discussion today, we may want to change the default so that you don't need to specify the --create-manifest-if-needed flag. For completeness we would probably add a flag to disable this behavior. If the --create-manifest-if-needed flag is not specified, then we probably do not want to create a manifest in the current folder; we would require there to be a .git or .sln file/folder.

@JL03-Yue JL03-Yue requested a review from nagilson March 27, 2023 19:38
@nagilson
Copy link
Member

nagilson commented Mar 28, 2023

@baronfel Was a final decision reached on this change? I believe we are going to go with what this PR does for now?


There is the proposed --manifest-file, but that was not added here to my knowledge.


In addition, regarding:

--create-manifest-if-needed false

...

change the default so that you don't need to specify the --create-manifest-if-needed

I assume that with the below stipulation, this means it's by default IFF the following is true?

If the --create-manifest-if-needed flag is not specified, then we probably do not want to create a manifest in the current folder

Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

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

Looks great! A few suggestions still.

@baronfel
Copy link
Member

baronfel commented Feb 20, 2024

@JL03-Yue we need to doc this! I just went looking and couldn't find it. I'll make a docs issue and we can work on doing that.

Logged dotnet/docs#39600 for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants