-
Notifications
You must be signed in to change notification settings - Fork 719
Enhance dotnet.generateAssets command to accept skipPrompt parameter for asset generation #8510
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
…for asset generation
Co-authored-by: Andrew Wang <waan@microsoft.com>
Co-authored-by: Andrew Wang <waan@microsoft.com>
@WardenGnaw put everything you suggested! Let me know if it's ok now |
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.
LGTM provided @WardenGnaw is good with these updates. So please wait for his approval.
Also tagging @dibarbet and @JoeRobich to see if they have any concerns.
Co-authored-by: Andrew Wang <waan@microsoft.com>
@@ -53,8 +53,8 @@ export function registerDebugger( | |||
); | |||
|
|||
context.subscriptions.push( | |||
vscode.commands.registerCommand('dotnet.generateAssets', async (selectedIndex) => { | |||
if (!(await promptForDevKitDebugConfigurations())) { | |||
vscode.commands.registerCommand('dotnet.generateAssets', async (selectedIndex: number, options: { skipPrompt?: boolean } = {}) => { |
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.
I assume the expectation is that the container tools extension is calling this programatically and can pass in the value they want here?
Additionally, generate assets can be called from the command palette directly, but generally command palette options don't pass in arguments. Can you verify the generate assets command still works there?
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.
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.
Yep. The arguments shouldnt matter via command since its only used for calling it via the command API.
We already do this today with selectedIndex
, now we are just creating another parameter for options
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.
Yep. The arguments shouldnt matter via command since its only used for calling it via the command API.
However the command is exposed today as a user-invokable command. Should we remove that?
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.
Its intended to be invokable, and if users have DevKit installed, they should see the prompt.
@claudiaregio You will need to run the linter with |
Bug: #8414