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

If the config looks functional return it, otherwise force VSCode to open config file #2558

Merged
merged 4 commits into from
Oct 2, 2018
Merged

Conversation

isidorn
Copy link
Contributor

@isidorn isidorn commented Sep 27, 2018

Hi VSCode dev here,

VSCode changed the functionality when to automatically open a launch.json file.
Previosuly if the return configuraiton would not have a .type field we would automatically open it, however we decided that the extensions must explictly request this by returning null. More details about the change and the motivation behind it can be found here
microsoft/vscode#54213

It would be great if we could merge this as soon as possible since it creates a regression, more details about it here microsoft/vscode#59487

fyi @gregg-miskelly @DustinCampbell @akshita31
sorry for pinging everyone

@isidorn
Copy link
Contributor Author

isidorn commented Sep 27, 2018

Also this PR will not break the existing vscode stable.
The next vscode stable will be released in a week and ideally we could merge this before than. If that is not an option please let me know.

fyi @pieandcakes to check if C++ is also relying on this behavior, if yes I can provide a PR also in that repo.

@codecov
Copy link

codecov bot commented Sep 27, 2018

Codecov Report

Merging #2558 into master will increase coverage by 0.07%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2558      +/-   ##
==========================================
+ Coverage   65.04%   65.11%   +0.07%     
==========================================
  Files          98       98              
  Lines        4248     4248              
  Branches      612      613       +1     
==========================================
+ Hits         2763     2766       +3     
+ Misses       1306     1305       -1     
+ Partials      179      177       -2
Flag Coverage Δ
#integration 53.17% <0%> (+0.07%) ⬆️
#unit 84.73% <ø> (ø) ⬆️
Impacted Files Coverage Δ
src/configurationProvider.ts 19.64% <0%> (-1.79%) ⬇️
src/omnisharp/server.ts 70.68% <0%> (-0.69%) ⬇️
src/features/diagnosticsProvider.ts 76.81% <0%> (+1.44%) ⬆️
src/features/codeLensProvider.ts 48.54% <0%> (+1.94%) ⬆️
src/omnisharp/delayTracker.ts 73.68% <0%> (+5.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44536b3...fec2fad. Read the comment docs.

@@ -140,9 +140,9 @@ export class CSharpConfigurationProvider implements vscode.DebugConfigurationPro
config = this.parseEnvFile(config.envFile.replace(/\${workspaceFolder}/g, folder.uri.path), config);
Copy link
Contributor

Choose a reason for hiding this comment

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

@isidorn Will VS Code sometimes call this function with a null or undefined config argument, and so this if statement should be if (config && config.envFile)?

@isidorn
Copy link
Contributor Author

isidorn commented Oct 2, 2018

@gregg-miskelly @WardenGnaw Hi guys. Just a friendly reminder to merge this if possible. We are soon to release a new vscode stable build and it would be cool if this change is part of your extension before that.

@gregg-miskelly
Copy link
Contributor

@isidorn I had one question that I was hoping you would answer before I merged this.

@isidorn
Copy link
Contributor Author

isidorn commented Oct 2, 2018

@gregg-miskelly sorry! Somehow I missed the question. No vscode should never call that with a null or undefined config. Config should be an empty object at worst. Code pointer
If you are seeing null or undefined than that is a bug which shuold be fixed on the vscode side.

@gregg-miskelly
Copy link
Contributor

Cool. Thanks for confirming. I will commit this as soon as the build rules are satisfied.

@gregg-miskelly gregg-miskelly merged commit 37c0fa1 into dotnet:master Oct 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants