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

handle user cancellation for variables #11406

Merged

Conversation

paul-marechal
Copy link
Member

@paul-marechal paul-marechal commented Jul 11, 2022

Allow a variable to throw a cancellation error to notify the variable
resolver that the resolution process should be aborted.

Remove checkAllResolved option from VariableResolveOptions.

Closes: #11332

How to test

  1. Download and build the following test project
    https://github.com/alvsan09/hello_express

  2. Open the project in theia

  3. From the debug view launch the configuration Attach to node process,

    • Verify that a list of processes is present
    • Click outside the list area or the escape key
    • Make sure that not Error dialogs pop up
  4. Launch the configuration Launch Backend

  5. Launch the configuration Launch Frontend

    • Verify that a chrome browser opens successfully with the Hello World message.

launch_with_undefined_variables

Review checklist

Reminder for reviewers

Allow a variable to throw a cancellation error to notify the variable
resolver that the resolution process should be aborted.

Remove `checkAllResolved` option from `VariableResolveOptions`.
@paul-marechal paul-marechal requested a review from alvsan09 July 11, 2022 14:55
@paul-marechal paul-marechal force-pushed the mp/variable-cancellation branch 2 times, most recently from 5580211 to 1ecf161 Compare July 11, 2022 15:12
@paul-marechal paul-marechal force-pushed the mp/variable-cancellation branch from 1ecf161 to 75e82fe Compare July 11, 2022 15:14
Copy link
Contributor

@alvsan09 alvsan09 left a comment

Choose a reason for hiding this comment

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

@paul-marechal ,
it seems to work fine! 👍
As per the code, only a couple of cosmetics as inline comments!!

@paul-marechal paul-marechal marked this pull request as ready for review July 11, 2022 19:16
@paul-marechal
Copy link
Member Author

paul-marechal commented Jul 11, 2022

@jfaltermeier regarding your comment I agree that our current components lack something. I think we should address that as a follow-up.

Copy link
Contributor

@alvsan09 alvsan09 left a comment

Choose a reason for hiding this comment

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

I have tested as per instructions and it works fine 👍

The code LGTM !!

@vince-fugnitto vince-fugnitto added the variable-resolver issues related to the variable-resolver extension label Jul 12, 2022
@paul-marechal paul-marechal merged commit e44aac7 into eclipse-theia:master Jul 13, 2022
@paul-marechal paul-marechal deleted the mp/variable-cancellation branch July 13, 2022 17:41
@vince-fugnitto vince-fugnitto added this to the 1.28.0 milestone Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
variable-resolver issues related to the variable-resolver extension
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants