-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[WIP] dotnet tool exec <package[@version]> one-shot execution
#48443
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
Conversation
|
Commented offline that we wanted to start with "dotnet tool run --from-source". We're currently discussing alias options or auto-detection options but the full command is safest to start with. |
|
@edvilme I would recommend patterning this more off of local tools than global tools. Global tools download the packages to a folder structure under Local tools on the other hand run directly from the packages in the NuGet global packages folder. I think it would be easier to run a tool from there without any side effects besides the fact that the package is now in the global packages folder. |
nagilson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things to consider for review when it is finished / good test cases:
- Does it delete the tool, as it says without permanently installing it
- It passes the args to tools
- Version resolution works
- Uses latest if no version specified
- It uses the feed
- Make sure we prompt before downloading something new
- Does it have -y -n
- Supports nuget feed options
I like dotnet toolx personally but ok to see why we are using --from-source for now since it matches the other patterns.
|
/ba-g stuck |
|
Note: Ready for review, failing some test. |
Forgind
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The core functionality here looks good to me, but there are several cleanup things I noticed as I was reviewing. Great work!
| }; | ||
|
|
||
| public static readonly Option<string> VersionOption = ToolInstallCommandParser.VersionOption; | ||
| public static readonly Option<bool> RollForwardOption = ToolInstallCommandParser.RollForwardOption; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have to define these here as opposed to just adding ToolInstallCommandParser.RollForwardOption in ToolExecuteCommandParser.ConstructCommand and using that in the Command?
src/Cli/dotnet/Commands/Tool/Execute/ToolExecuteCommandParser.cs
Outdated
Show resolved
Hide resolved
|
I fixed the issues flagged in the PR review from forgind and did some local testing. Testing done:
Testing I haven't done:
I didn't review the code or the automated tests yet. 2 issues identified:
|
dotnet/command-line-api#2564 went into S.CL last week that you're intended to use for this scenario - kind of a marker that "trust me bro, I know what I'm doing" |
| <data name="YesOptionDescription" xml:space="preserve"> | ||
| <value>Overrides confirmation prompt with "yes" value. </value> | ||
| </data> | ||
| <data name="NoOptionDescription" xml:space="preserve"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still seems unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in follow-up PR: #49329
| <data name="ToolRunCommandDescription" xml:space="preserve"> | ||
| <value>Run a local tool. Note that this command cannot be used to run a global tool. </value> | ||
| </data> | ||
| <data name="ToolRunArguementsDescription" xml:space="preserve"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tiny nit:
| <data name="ToolRunArguementsDescription" xml:space="preserve"> | |
| <data name="ToolRunArgumentsDescription" xml:space="preserve"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in follow-up PR: #49329
|
I've created a new PR to continue work on this, so I'm going to close this one. |
Closes #31103
WIP: Name, and implementation may change