-
Notifications
You must be signed in to change notification settings - Fork 646
Add go.toolCommands
which specifies alternate command locations
#1297
Conversation
89e20b7
to
3d8a1f0
Compare
7d591f1
to
a8a68e2
Compare
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.
@ikedam Thanks for this PR and I apologize for not getting to this earlier.
I found 1 issue due to which this will not work in Windows and another due to which debugging will not work.
I will push a commit to fix both, can you help with testing?
src/goPath.ts
Outdated
|
||
let binPathCache: { [bin: string]: string; } = {}; | ||
let runtimePathCache: string = ''; | ||
|
||
export function getBinPathFromEnvVar(toolName: string, envVarValue: string, appendBinToPath: boolean): string { | ||
toolName = correctBinname(toolName); | ||
let binname = correctBinname(toolName); |
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.
correctBinName
gets run twice resulting in strings like golint.exe.exe
in Windows
src/goPath.ts
Outdated
@@ -11,16 +11,21 @@ | |||
import fs = require('fs'); | |||
import path = require('path'); | |||
import os = require('os'); | |||
import vscode = require('vscode'); |
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.
This file is used by the debug adapter via the goDebug.ts
file. The debug adapter runs in a different process than the extension host and so will not have access to vscode
To test this feature, do the following:
Add the setting |
Downloaded and installed this from the .vsix file. It seems to be doing what it says on the tin: When I add the following to my settings.json:
Then looking in the Output tab I see the tool being replaced properly in my project:
However: when I intentionally break my code by adding an unused import, I do not get any error report. In the terminal, running this manually I get:
But no output in VSCode or the Problems tab. I don't know if this is a separate issue or not. |
@ramya-rao-a |
@jlindsey Would you tell me what is In my environment, error reports are shown as expected by using The issue may be caused for what |
@ikedam https://github.com/skelterjohn/wgo It is similar to |
@ikedam @ramya-rao-a After doing some more tests on my end, I believe my results were caused by old configs in my global User settings. After removing all the LGTM now. 👍 |
Thanks for verifying @ikedam and @jlindsey On second thoughts, I am wondering if there are really any benefits of having this customization feature for other tools (not This feature provides 2 benefits. One is the ability to use a totally different tool, the other is to use the same tool from a different location. Each of these tools take in very specific input parameters. What are the chances of having another tool that takes in exact same parameters? The benefit of different location is available even today. If you have the tool under the PATH variable, then it will get picked up. I propose to have this customization feature just for Thoughts? |
This is fine with me and it's all I personally need. However, one benefit of |
Correct me if I'm wrong, but right now the feature seems to be completely generic. It's a mapping that just works for all tools. Would limiting it to "go" not require adding extra checks just to take away features? As for the usefulness for other tools, one issue I've run into several times already is that to figure out why something wasn't working (most recently why gogetdoc wasn't finding the docs for a symbol) I introduced a wrapper script that would log the parameters the tool was called with and its output. This feature would make this much easier and cleaner. Adding a script with the same name as a standard tool to a directory in the PATH is a clutch. With this feature I could do such testing in workspace settings so it wouldn't interfere with anything. And then there is the case of my goformat tool. Before it was added to the valid list of values for go.formatTool, I had to rename it to gofmt to avoid the error. Again, this mapping feature would have provided a cleaner solution. Right now I'm toying with the idea of developing an alternative to gogetdoc that works without compiling, so it would be faster. This mapping feature would make testing this tool easier because I could override gogetdoc in the workspace settings as opposed to fiddling with my PATH. To put it simply: There are a lot of fringe uses for a generic mapping feature like this. If it doesn't make the code worse, I would keep the ability to remap all tools. |
Speaking of my use cases, if this doesn't work already, please add the ability to use the ${workspaceFolder} variable in the mapping for a tool, so that I can have mappings in my workspace settings together with the tools they refer to. |
@ramya-rao-a +1 as
-1 as
So how about going in this way?:
I think I can work for that in June. |
I worry that we lose the chance to generalize |
I'll have to go back in history to see why the two were implemented differently to begin with :) So that we dont regress anything |
This is merged in master. Please give it a try by following the below steps and share any feedback you have.
|
@ramya-rao-a Downloaded and installed successfully. Switched my config above for the new setting name. Everything still works fine and as expected. Thanks for your hard work on this! |
This feature is now available in the latest update to the Go extension (0.6.83) |
@ramya-rao-a I tried 0.6.83-beta-1 (and 0.6.83, sorry for my late response). |
Thanks to you too @ikedam! |
Appengine Go SDK provides
goapp
command which wrapsgo
command.I need to create a symbolic link
go
togoapp
to develop Appengine Go applications with vscode-go.But I don't want to do that as it masks the original
go
command. I want to use bothgoapp
in the Appengine Go SDK andgo
of of the Appengine Go SDK.This request enables to configure alternate command for
go
like:You can also provide absolute path:
You can also configure alternate command for other tool: