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

Bail out from launch for unresolvable variables #44411

Closed
weinand opened this issue Feb 26, 2018 · 10 comments
Closed

Bail out from launch for unresolvable variables #44411

weinand opened this issue Feb 26, 2018 · 10 comments
Assignees
Labels
debt Code quality issues debug Debug viewlet, configurations, breakpoints, adapter issues release-notes Release notes issues verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@weinand
Copy link
Contributor

weinand commented Feb 26, 2018

Typical case:

  • have a default launch config that uses a ${file} variable for the program attribute
  • close all editors in VS Code
  • press F5

Observe: you get the follwoing error:
2018-02-26_12-53-08

@weinand weinand added debug Debug viewlet, configurations, breakpoints, adapter issues debt Code quality issues labels Feb 26, 2018
@weinand weinand added this to the March 2018 milestone Feb 26, 2018
@isidorn
Copy link
Contributor

isidorn commented Feb 26, 2018

I will add an option in the configurtionResolverService if something can not be resolved to bail out. Tasks will continue behaving like they do now. @dbaeumer let me know if in debt week you want me to turn this on for tasks as well so we get some user feedback and figure out if this is an improvement

@weinand
Copy link
Contributor Author

weinand commented Feb 26, 2018

I suggest that the configurationResolverService just throws an error and that the debug launch UI turns that into an appropriate alert, e.g. something helpful for ${file}: "no editor open" and for ${workspaceFolder}: "folder qualifier missing".

@weinand
Copy link
Contributor Author

weinand commented Feb 26, 2018

One complication is the fact that some debug extensions introduce variables themselves (without declaring them in the package.json) and resolve them on their end. In this case we should neither touch them nor flag them as errors.

@isidorn
Copy link
Contributor

isidorn commented Mar 7, 2018

We will only flag as errors our pre defined list of variables. We do not touch extension introduced variables.
All error messages will come from the configurationResolverService since those errors are true for whatever client of the service, they are not debug specific. Also I do not want to be in the error massaging business in debug land.

@isidorn isidorn closed this as completed in f2584c3 Mar 7, 2018
@isidorn
Copy link
Contributor

isidorn commented Mar 7, 2018

I have added nice error message (can be seen in the attached commit)
Feedback on wording is very welcome.

env variables will not throw an error if they can not be found

@dbaeumer please note that I have changed this behavior for all configurationResolverService clients including Tasks. Let's try it out for a couple of days and if we decide that Tasks want it to not bail out with an error I will add an option. But first wanted to try to be consistent.

@isidorn isidorn added verification-needed Verification of issue is requested release-notes Release notes issues labels Mar 26, 2018
@weinand weinand reopened this Mar 28, 2018
@weinand
Copy link
Contributor Author

weinand commented Mar 28, 2018

Works for launch configs in user settings but nowhere else.

Yes, in my example I was referring to a "default launch config" but the feature should work everywhere.

In addition the error notification should be modal, e.g. like the following:

2018-03-28_15-57-04

(BTW, this is the misleading error I'm receiving when there is a typo in the variable ${workspaceRoot}; instead I would like to see your new message)

@weinand weinand added the verification-found Issue verification failed label Mar 28, 2018
@isidorn
Copy link
Contributor

isidorn commented Mar 28, 2018

@weinand I have pushed a change that an unkown variable also throws an error.
For using model dialogs that is a bit larger change and I would ask you to please create a debt item for me.
In some cases we do use modeal dialogs, but in this partiucalr case no

@weinand
Copy link
Contributor Author

weinand commented Mar 28, 2018

@isidorn I'm not sure whether your fix for unknown variables is really a good idea: above you said

We will only flag as errors our pre defined list of variables. We do not touch extension introduced variables.

We'll have to be careful not to block any launch config that contains a variable that you cannot resolve, but which is privately used by a DA.

My issue from above was actually something else: I got the impression that your bailing out for a unresolvable "${file}" would only work in a default launch config coming from the user settings, but not in a normal launch config from a launch.json.

But I just tried it again and I'm no longer seeing that problem.

Bottom line:

  • don't flag "unkown variables" too eagerly (or just revert that fix),
  • fix the missing quotes
  • try to make the error modal now (but if this is too complex do it in April).

@isidorn
Copy link
Contributor

isidorn commented Mar 28, 2018

Makes sense.

  • Reverted
  • Fixed quotes
  • Will look into later, if I do not have time will craete an issue for april

@isidorn
Copy link
Contributor

isidorn commented Mar 28, 2018

Created #46870

@weinand weinand added verified Verification succeeded and removed verification-found Issue verification failed labels Mar 30, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators May 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues debug Debug viewlet, configurations, breakpoints, adapter issues release-notes Release notes issues verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

2 participants