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

Added showGlobalVariables as a workspace level parameter #2351

Merged
merged 5 commits into from
Mar 2, 2019

Conversation

aschade92
Copy link
Contributor

Before, this option was only available in launch.json but this PR makes showGlobalVariables as a workspace level setting so that things like the debug and run test actions can use this setting.

…n package.json and also updated goDebugConfiguration.ts to parse the new showGlobalVariables
@msftclas
Copy link

msftclas commented Feb 25, 2019

CLA assistant check
All CLA requirements met.

@aschade92
Copy link
Contributor Author

#2323

package.json Outdated
@@ -1233,7 +1238,8 @@
"maxVariableRecurse": 1,
"maxStringLen": 64,
"maxArrayValues": 64,
"maxStructFields": -1
"maxStructFields": -1,
"showGlobalVariables": false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we need to pull showGlobalVariables one level up.
The properties in LoadConfig is a 1:1 to what is at https://github.com/go-delve/delve/blob/4c9a72e486f1f0d0c90ecede8415a871dced8117/service/api/conversions.go#L276

@aschade92
Copy link
Contributor Author

@ramya-rao-a updated

@ramya-rao-a
Copy link
Contributor

@jhendrixMSFT FYI, I am changing the default value for showGlobalVariables to false as it is still causing perf issues for some users and the benefits of having it enabled doesnt outweigh the issues.

@ramya-rao-a
Copy link
Contributor

@aschade92 Looks like this is your first PR contribution to this project, Thanks & Welcome!

I have pushed 2 more commits. 1 to update the description and default value for the dlvLoadConfig setting, the other is explained at #2351 (comment)

@ramya-rao-a ramya-rao-a merged commit e19013a into microsoft:master Mar 2, 2019
@aschade92 aschade92 deleted the workspace-show-global-vars branch March 3, 2019 17:29
@aschade92
Copy link
Contributor Author

@ramya-rao-a sounds good, thanks for reviewing!

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.

3 participants