Add dotnet tool exec command for one-shot tool execution#49329
Add dotnet tool exec command for one-shot tool execution#49329dsplaisted merged 45 commits intodotnet:mainfrom
dotnet tool exec command for one-shot tool execution#49329Conversation
| </data> | ||
| <data name="ToolDownloadConfirmationPrompt" xml:space="preserve"> | ||
| <value>Tool package {0}@{1} will be downloaded from source {2}. | ||
| Proceed? [y/n]</value> |
There was a problem hiding this comment.
Perhaps we could reword this to be more localization friendly. Something like Press 'y' to proceed or 'n' to decline. This way, we can translate proceed and decline, and lock the y/n keys from being localized. I imagine for non-Latin keyboards, we may need to translate the Y/N as well to display.
There was a problem hiding this comment.
I think this is a good suggestion, but I personally prefer to be less verbose here.
src/Cli/dotnet/CliStrings.resx
Outdated
| <comment>{Locked="--version"}</comment> | ||
| </data> | ||
| <data name="YesOptionDescription" xml:space="preserve"> | ||
| <value>Overrides confirmation prompt with "yes" value. </value> |
There was a problem hiding this comment.
Should "yes" not be translated? It sounds like a specific, non-localized value.
There was a problem hiding this comment.
I've updated how the confirmation prompt works and is localiized. I think the yes string here can be localized.
src/Cli/dotnet/Commands/Tool/Execute/ToolExecuteCommandParser.cs
Outdated
Show resolved
Hide resolved
src/Cli/dotnet/Commands/Tool/Execute/ToolExecuteCommandParser.cs
Outdated
Show resolved
Hide resolved
| toolExecutable, | ||
| commandArguments); | ||
| } | ||
| else if (toolRunner == "executable") |
There was a problem hiding this comment.
nit: Redundant else usage (both this else-if and the plain else).
There was a problem hiding this comment.
It's redundant but seems a bit clearer to me like this. If you get rid of the elses you have to pay attention to the return statements inside the blocks to understand the overall flow.
| </data> | ||
| <data name="YesOptionDescription" xml:space="preserve"> | ||
| <value>Overrides confirmation prompt with "yes" value. </value> | ||
| </data> |
There was a problem hiding this comment.
| </data> | |
| <comment>{Locked="yes"}</comment> | |
| </data> |
There was a problem hiding this comment.
I don't actually think this needs to be locked, I've updated the confirmation prompt to support localization better.
There was a problem hiding this comment.
Doesn't accept imply yes (whereas decline implies no)?
dsplaisted
force-pushed
the
dotnet-tool-exec
branch
from
05ef93b to
4bbafef
Compare
|
|
||
| if (!_interactive) | ||
| { | ||
| return false; |
There was a problem hiding this comment.
Good call to default to 'no' in this instance!
There was a problem hiding this comment.
Per @baronfel, npx does it the other way 😀. So we still might change it.
There was a problem hiding this comment.
Ah, that actually makes sense. No is better than no default, but I think we should change it too. It would make scripts more verbose. I don't see a scenario in CI where I'd write this but want to say no.
Co-authored-by: Michael Yanni <MiYanni@microsoft.com>
dsplaisted
force-pushed
the
dotnet-tool-exec
branch
from
a46949c to
ccde6b0
Compare
nagilson
left a comment
nagilson
left a comment
There was a problem hiding this comment.
Thanks for sprucing up the code in some places. I read through everything and couldn't find any issues in the logic with the time we have before preview 6 snaps. All of the remaining feedback seems like nits to me - good feedback yet not blocking. Very excited to see this feature rollout :)
This is a continuation of this PR: #48443
Design doc: dotnet/designs#334
Original issue: #31103
Updates from previous PR include: