Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace settings variables in debugger config #8042 #8784

Merged
merged 2 commits into from
Jul 19, 2016
Merged

replace settings variables in debugger config #8042 #8784

merged 2 commits into from
Jul 19, 2016

Conversation

DonJayamanne
Copy link
Contributor

Provide the ability to expose (share) settings from the settings.json file to the launch.json file, using the dollar notation. #8042

settings.json:

"python.pythonPath": "/Library/Frameworks/Python.framework/Versions/3.5/bin/python3"

launch.json:

"pythonPath": "${settings.python.pythonPath}",

@mention-bot
Copy link

@DonJayamanne, thanks for your PR! By analyzing the annotation information on this pull request, we identified @isidorn and @egamma to be potential reviewers

@isidorn isidorn self-assigned this Jul 6, 2016
@isidorn isidorn added this to the July 2016 milestone Jul 6, 2016
}

protected resolveString(value: string): string {
let regexp = /\$\{settings\.(.*?)\}/g;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer if you just read the configuration every time you need to resolve the string.
Not sure if doing it the current way you can end up with a stale configuration.

@isidorn
Copy link
Contributor

isidorn commented Jul 6, 2016

@DonJayamanne PR looks good overall - I added comments in the commits directly. Also:

  • Did you test this both on the folder and no-folder workspace?
  • Can you please squash all the commits as the first 4 commits in this PR should be removed?
  • It would be great if this also worked for tasks.json
  • Is it possible that SettingsVariables just extends from SystemVariables - using that approach we would not need both SettingsVariables and SystemVariables in DebugConfigurationManager and we would get this feature for free in the tasks world?
  • We should not call this 'setting.' but 'config.' to align with other places where we have this

Adding @weinand

@DonJayamanne
Copy link
Contributor Author

@isidorn, @weinand

Did you test this both on the folder and no-folder workspace?

I have only tested this with a folder workspace.
Didn't know we can have VS Code with a no-folder workspace. How does that work?

Can you please squash all the commits as the first 4 commits in this PR should be removed?

Will do.

It would be great if this also worked for tasks.json
Is it possible that SettingsVariables just extends from SystemVariables - using that approach we would not need both SettingsVariables and SystemVariables in DebugConfigurationManager and we would get this feature for free in the tasks world?

Great idea. However, wouldn't this render SystemVariables obsolete? From what I can see this is only used in two places, running tasks and debug adapters.

Also it could be a little misleading, if we used this new class SettingsVariables where ever SystemVariables was being used, then one would be mistaken into assuming that only Settings (config) variables were being replaced.

I think this all boils down to the fact that this SettingsVariables (having extended SystemVariables) class will be doing two things:

  • Replacing System variables (base class functionality)
  • Replacing Settings (config) variables (overridden method functionality).

If you agree with the above then, how about we proceed with the following approach:

  • SettingsVaraibles extends SystemVariables (as per the suggestion)
  • Replace references to SystemVaraibles and SettingsVariables with ISystemVariables
  • Get ISystemVariables passed in as a constructor argument resolved via DI (resolved using the serviceCollection - no sure what the right terminology is here)

We should not call this 'setting.' but 'config.' to align with other places where we have this

Agreed, for now I'll continue using the word "Setting" and SettingsVariables so there's no confusion.

@isidorn
Copy link
Contributor

isidorn commented Jul 7, 2016

@DonJayamanne no folder workspace is the purple vscode - the window which you get when you say 'new window'. In that scenario we do not support debugging but just open that window and check for exceptions - since a lot of things are missing in that scenario.

Your proposed approach sounds good to me, can you code it up and then we can provide more comments?

Thanks!

@@ -169,8 +168,8 @@ export class ConfigurationManager implements debug.IConfigurationManager {
@IQuickOpenService private quickOpenService: IQuickOpenService,
@IKeybindingService private keybindingService: IKeybindingService
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will probably throw an exception if you open the no-folder workspace.
That is the reason why I originally check for this.contextService.getWorkspace(), I think you need to re-introduce that check.

@isidorn
Copy link
Contributor

isidorn commented Jul 8, 2016

@DonJayamanne pr looks great! There are only two things which should be updated - I commented in the code directly!

@DonJayamanne
Copy link
Contributor Author

@isidorn , please check again, Thanks

@isidorn
Copy link
Contributor

isidorn commented Jul 19, 2016

@DonJayamanne looks great! Sorry for the slow response, last week I was on vacation -> merging

@isidorn isidorn merged commit 971ed27 into microsoft:master Jul 19, 2016
@isidorn
Copy link
Contributor

isidorn commented Jul 20, 2016

@DonJayamanne please create a test plan item for this so we can test it next week in the endgame - thanks!

@DonJayamanne
Copy link
Contributor Author

DonJayamanne commented Jul 21, 2016

@isidorn, done, please check the test plan (#9560) and let me know how it looks.

@isidorn
Copy link
Contributor

isidorn commented Jul 21, 2016

@DonJayamanne looks great! I just assigned it to the current milestone (july) so it is easier for the endgame master to find it. I would also mention to try it for tasks.json as well.
My personal preference is to write more concise test plan items so the tester is given more freedom as to how to exactly test the new feature. But that is up to you

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.

5 participants