Provide func tasks programmatically #936
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Jan 25, 2019
|
ping |
nturinski
approved these changes
Jan 31, 2019
| private readonly _pythonDebugProvider: PythonDebugProvider; | ||
| private readonly _javaDebugProvider: JavaDebugProvider; | ||
|
|
||
| constructor(nodeDebugProvider: NodeDebugProvider, pythonDebugProvider: PythonDebugProvider, javaDebugProvider: JavaDebugProvider) { |
There was a problem hiding this comment.
Why have this be part of the constructor rather than just export it from the respective files? It doesn't seem like there are any dynamic properties in the constructors for any of the DebugProviders.
| if (venvName) { | ||
| command = convertToVenvCommand(venvName, process.platform, command); | ||
| } | ||
| const options: ShellExecutionOptions = { env: { languageWorkers__python__arguments: await getPythonCommand(localhost, port) } }; |
There was a problem hiding this comment.
Was the Python extension supposed to handle activating the virtual environment as well? I thought that was part of the reason we wanted them to provide an API.
| context.subscriptions.push(vscode.debug.registerDebugConfigurationProvider('node', nodeDebugProvider)); | ||
| context.subscriptions.push(vscode.debug.registerDebugConfigurationProvider('python', pythonDebugProvider)); | ||
| context.subscriptions.push(vscode.debug.registerDebugConfigurationProvider('java', javaDebugProvider)); | ||
| context.subscriptions.push(vscode.workspace.registerTaskProvider(func, new FuncTaskProvider(nodeDebugProvider, pythonDebugProvider, javaDebugProvider))); |
There was a problem hiding this comment.
Is the C# launch too simple to justify creating a FuncTaskProvider for it?
|
Discussed offline. Might add more stuff for the C# scenario in the future, but not needed right now. |
ejizba
added a commit
that referenced
this pull request
Jan 31, 2019
I was having problems with tests when I tried to run them against Insiders for this PR: #936 These are just a few quality of life improvements that came out of that. 1. Use `mocha-multi-reporters` like stephen's done in other repos 1. Set `ext.ui` immediately. In some cases this will report "Unexpected call to showWarningMessage..." instead of a timeout 1. Added `runWithSetting` so that I had more control on when settings are used in tests. For example, if the `dispose` method of `FunctionTesterBase` failed at `fse.remove` these weren't getting set back
This was referenced Feb 4, 2019
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By providing func tasks programmatically, we get the following benefits:
I'm somewhat hesitant to merge this PR to master because it relies on a few fixes only in insiders, but I think it's worth it to give it some bake time since it affects a high priority scenario (debug) within our extension.
Fixes #822
Fixes #821
I've also filed a few issues for remaining work: #932 #933 #935