Run Button for Circuit Files - Standalone and Project #2455
Conversation
…oth project and standalone scenarios
…s no way to get the circuit file this way
| } | ||
|
|
||
| function registerCommands(context: vscode.ExtensionContext) { | ||
| // Register commands for running and debugging Q# files. | ||
| context.subscriptions.push( | ||
| vscode.commands.registerCommand( | ||
| `${qsharpExtensionId}.runEditorContents`, | ||
| `${qsharpExtensionId}.runQsharp`, |
There was a problem hiding this comment.
If we're renaming, may as well make it as accurate as possible - what about runProgram? We can run more than Q# with this command (i.e. qasm)
| "command": "qsharp-vscode.runEditorContents", | ||
| "when": "resourceLangId == qsharp || resourceLangId == openqasm" | ||
| "command": "qsharp-vscode.runCircuitContents", | ||
| "when": "false" |
There was a problem hiding this comment.
Why when: false? Why have this command here at all if it's never enabled?
| (resource: vscode.Uri, expr?: string) => { | ||
| // if expr is not a string, ignore it. VS Code can sometimes | ||
| // pass other types when this command is invoked via UI buttons. | ||
| if (typeof expr !== "string") { | ||
| expr = undefined; | ||
| } | ||
| startQdkDebugging(resource, { | ||
| name: "Debug File", | ||
| name: "Debug", |
There was a problem hiding this comment.
Again, while we're renaming - ideally these names would be more descriptive and would more uniquely identify our scenario. They are visible to the user and correspond to the name field in launch.json. Perhaps: "QDK: Debug program"
| } | ||
|
|
||
| // Returns true if any ancestor directory (excluding the file's own directory) contains qsharp.json | ||
| async function checkIfInQsharpProject(uri: vscode.Uri): Promise<boolean> { |
There was a problem hiding this comment.
Didn't we talk about using findManifestDocument here instead of reimplementing the qsharp.json search logic?
| if ( | ||
| document.languageId === "qsharp" || | ||
| document.languageId === "openqasm" || | ||
| document.languageId === "qsharpcircuit" | ||
| ) { |
There was a problem hiding this comment.
Please prefer isQdkDocument instead of spot checking languageId like this - it's a more foolproof check
There was a problem hiding this comment.
Thought of one more thing - checking for QASM here is kind of meaningless since .qasm files and qsharp.json files never go together. Like a .qasm file could never be in a qsharp.json project. So you may want to tweak this check to use the more appropriate helpers from common.ts
| vscode.commands.executeCommand( | ||
| "setContext", | ||
| "qsharp.isProjectFile", | ||
| isProjectFile, |
There was a problem hiding this comment.
As long as we're setting a context key, I feel like we could make it something more helpful than just a boolean. Can we call this, say, activeQSharpProjectRoot and set it to the path of the active project directory (and null if we're not in a project)? This way, it's a bit more debuggable than true/false if something goes wrong and we don't understand what's happening.
| let namespaceName: string | undefined = undefined; | ||
|
|
||
| try { | ||
| const program = await loadProject(resource); |
There was a problem hiding this comment.
You probably don't need the whole loadProject functionality here (which can be quite heavyweight especially if you have dependencies) since you just need the manifest path. Consider findManifestDocument
There was a problem hiding this comment.
Let's discuss design before proceeding with this. In general, I'm not sure the Run / Debug drop-down is the right place to add this functionality. compared to just a "Run" button in the editor or something. I'm not sure putting it in there is that discoverable or convenient (I'm guessing/hoping F5 still runs the project and not the circuit?)
Does it integrate with actual debugging (i.e. can you step through the circuit and see quantum state evolve)?
| @@ -146,24 +146,33 @@ | |||
| "menus": { | |||
| "editor/title/run": [ | |||
| { | |||
| "command": "qsharp-vscode.runEditorContents", | |||
| "when": "resourceLangId == qsharp || resourceLangId == openqasm", | |||
| "command": "qsharp-vscode.runQsharp", | |||
There was a problem hiding this comment.
Why did all these get changed to *Qsharp from *Editor though? These apply to OpenQASM files also now, not just qsharp.
| @@ -43,13 +43,11 @@ suite("OpenQASM Debugger Tests", function suite() { | |||
| vscode.debug.removeBreakpoints(vscode.debug.breakpoints); | |||
| }); | |||
|
|
|||
| test("Launch with debugEditorContents command", async () => { | |||
| test("Launch with debugQsharp command", async () => { | |||
There was a problem hiding this comment.
Yeah, I don't get why the command name changed, and we should change it back. This is an OpenQASM test and you invoke debugQsharp, which is weird.
| "command": "qsharp-vscode.debugEditorContents", | ||
| "when": "resourceLangId == qsharp || resourceLangId == openqasm" | ||
| "command": "qsharp-vscode.runQsharp", | ||
| "when": "resourceLangId == qsharp || resourceLangId == openqasm || (qsharp.isProjectFile && resourceLangId == qsharpcircuit)" |
There was a problem hiding this comment.
What do you need this isProjectFile context for? There's a lot of clunky code to set it on updates, and anyway the PR description says "This new functionality is available even if the circuit is not part of a Q# project"?
Adds the VS Code Run button to circuit files so that the Q# project they are a part of can be run or debugged while looking at a circuit file. Also adds a command that the Run button can use for running just the circuit file and producing a dump machine for the circuit. This new functionality is available even if the circuit is not part of a Q# project.