Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

added go.inferGopath. Similar to GoSublime's use_gs_gopath #762

Merged
merged 2 commits into from
Feb 3, 2017

Conversation

codmajik
Copy link
Contributor

@codmajik codmajik commented Feb 2, 2017

I would like to propose this implementation for the feature request raised in #757

@msftclas
Copy link

msftclas commented Feb 2, 2017

Hi @codmajik, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@codmajik codmajik changed the title added go.inferGopath. Similar to GoSublime's use_gs_gopath. see #757 added go.inferGopath. Similar to GoSublime's use_gs_gopath Feb 2, 2017
@msftgits
Copy link
Contributor

msftgits commented Feb 2, 2017

Hi, I am closing and re-opening this PR to bump the CLA bot. Sorry for the inconvenience!

@msftgits msftgits closed this Feb 2, 2017
@msftgits msftgits reopened this Feb 2, 2017
@msftclas
Copy link

msftclas commented Feb 2, 2017

Hi @codmajik, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!
We will now validate the agreement and then real humans will evaluate your PR.

TTYL, MSBOT;

@ramya-rao-a
Copy link
Contributor

@codmajik Thanks for the PR!

A few notes:

Take a case where the user has set go.gopath to a particular path, and also set go.inferGopath to true. Then according to your current logic:

  • the inferred path wins. I was wondering if this should be the other way around.
  • If the user then sets go.inferGopath to false, you will need to reload the VS Code window for the change to take effect. If we only infer the go path, when there is no go.gopath, then you won't need the reload

Also, can you write a unit test for this as well? You will have to change updateGoPathGoRootFromConfig to take the goConfig as a parameter to do that.

@codmajik
Copy link
Contributor Author

codmajik commented Feb 3, 2017

@ramya-rao-a Thanks for your feedback.

The case you provided is the actual use case for this and also the current implementation in GoSublime.

Where go.gopath is set in the global config and go.inferGopath is set in workspace config. Though
I am adding code to allow the reserve when go.inferGopath is global and go.gopath is set in workspace

@ramya-rao-a
Copy link
Contributor

@codmajik I am assuming that when you say "global config", you mean the "User settings" in VS Code.

All configurations defined in the package.json are available for changes in both Workspace and User settings.

By the time you fetch the settings by vscode.workspace.getConfiguration(), Workspace and User settings are already merged (workspace overrides user), so as an extension author, we have no idea if the value for a config was set in global/user settings or workspace settings.

Therefore, in our case here both go.gopath and go.infergopath can be set either in User or Workspace settings. We need to adapt for both.

Thinking more on this, it makes sense for the inferred path to win when both go.gopath and go.inferGopath are set.

@ramya-rao-a ramya-rao-a merged commit 94960c7 into microsoft:master Feb 3, 2017
@codmajik
Copy link
Contributor Author

codmajik commented Feb 4, 2017

@ramya-rao-a Thanks.

I made those changes on my local checkout. I know it is not needed but I can create a new pull request with that as improvements. I just need to figure out how to test this with your current test suite.

btw vscode.workspace.getConfiguration('go').inspect('gopath') would give you the User Setting and Workspace Setting separately.

@ramya-rao-a
Copy link
Contributor

@codmajik Never mind the tests, I was thinking of doing a major refactoring in the unit test area soon, I can take care of that.

What changes are you referring to exactly?

@codmajik
Copy link
Contributor Author

codmajik commented Feb 5, 2017

@ramya-rao-a, changes to make sure that if go.gopath is set in the workspace config and but go.inferGopath is set in User Settings then workspace go.path would take precedence.

PS: should I delete the branch?
PS 2: Does it make sense to add a notification for any of this

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Feb 6, 2017

@codmajik I think its fine the way it is. You can delete your branch.

I have updated the descriptions for both go.gopath and go.inferGopath to reflect that the latter overrides the former.

There is command Go: Current GOPATH that shows the GOPATH being used by the extension. It says: "Current GOPATH: ..."
We can update that to say "Current GOPATH is inferred from the workspace root: ..." when the GOPATH is inferred.

Not very important, but you can give that a try.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants