Added processCommand to attach config #2969
Conversation
Added processCommand to optionsSchema.json Added processCommand to resolve debugCOnfig
|
|
Codecov Report
@@ Coverage Diff @@
## master #2969 +/- ##
=======================================
Coverage 89.89% 89.89%
=======================================
Files 59 59
Lines 1583 1583
Branches 89 89
=======================================
Hits 1423 1423
Misses 149 149
Partials 11 11
Continue to review full report at Codecov.
|
src/configurationProvider.ts
Outdated
| if (config.request === "attach" && config.processCommand) { | ||
| const processCommand = config.processCommand.replace(/\${workspaceFolder}/g, folder.uri.fsPath); | ||
| delete config.processCommand; | ||
| return this.attachItemsProvider.getAttachItems() |
There was a problem hiding this comment.
return [](start = 12, length = 6)
nit: make this an async method and use await.
src/configurationProvider.ts
Outdated
| if (attachItem == null) { | ||
| throw new Error("Can't find process"); | ||
| } | ||
| config.processId = attachItem.id; |
There was a problem hiding this comment.
config.processId = attachItem.id; [](start = 20, length = 33)
We should probably fail if there are multiple processes with the command rather than picking the first.
src/configurationProvider.ts
Outdated
| .then(attachItems => { | ||
| const attachItem = attachItems.find(ai => ai.detail === processCommand); | ||
| if (attachItem == null) { | ||
| throw new Error("Can't find process"); |
There was a problem hiding this comment.
"Can't find process" [](start = 40, length = 20)
We might want to mention what we are looking for.
There was a problem hiding this comment.
Now throws Couldn't find process with command ${processCommand}
Better error message if the process can't be found Error message if process was found more than once Deleted processCommand in unitDebuggingOptions
|
@gregg-miskelly Let me know if that's good. I'm not too familiar with Mocha so I'm not entirely sure how to mock |
| "processCommand": { | ||
| "type": "string", | ||
| "description": "Attaches to a process with this command", | ||
| "default": "If this is use 'processId' and 'processName' should not be used." |
There was a problem hiding this comment.
There is a typo here. You want 'used' instead of 'use'.
But thinking about it more, you might want to instead use something like dotnet exec /example/path/to/program.dll to provide an example of the kind of thing we might want.
| } | ||
|
|
||
| if (foundAttatchItems.length > 1) { | ||
| throw new Error(`Find ${foundAttatchItems.length} processes with the command "${processCommand}"`); |
There was a problem hiding this comment.
How about instead "Unable to select process. Multiple processes found with command line "${processCommand}".
I am not an expert on it either, so there may be other options. But what I have always done is extract out the thing I want to unit test into its own class which takes appropriate inputs (ex: probably a process listing + input debug target in your case) and then I can unit test just the transformation without having to get into real object mocking. |
Attaching to a process can get quite annoying if you're using
dotnet watchasprocessNameisn't specific enough as there are multiple processes called dotnet.This pr add a config option called
processCommandto the attach config that will find the process by it's command.This pr is just a proof of concept, I'm just looking for feedback on the idea.
If you like the feature I'll do a proper one with all the unit testing and correct error messages, etc.