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

Bugfix/Stringify Task ENUMs for Presentation and PanelKind #7982

Merged

Conversation

kenneth-marut-work
Copy link
Contributor

What it does

This PR addresses #7956 where ENUM values for PanelKind and RevealKind were not properly being stringified to their values defined in the task-schema-updated.ts (referenced below) when using .setTaskConfigurations() to write tasks to a tasks.json file, causing an Invalid task configurations are found error :

enum: ['always', 'silent', 'never'],

enum: ['shared', 'dedicated', 'new'],

How to test

  • Create an extension that injects the TaskConfigurationManager
  • Build an object of type TaskConfiguration with some values (fill in proper scope/source value), use
    enum types for presentation panel and reveal, for example:
       const pendingTaskConfiguration: TaskConfiguration = {
            label: 'my task',
            type: 'shell',
            command: 'top',
            problemMatcher: [],
            args: '',
            _scope: 2,
            _source: scopeURI,
            options: {
                cwd: '',
            },
            presentation: {
                reveal: RevealKind.Always,
                panel: PanelKind.Shared
            },
        };
  • Attempt to call this.taskConfigurationManager.setTaskConfigurations(scopeURI, [pendingTaskConfiguration])

  • Open corresponding tasks.json file and observe that reveal and panel values have been properly set to their string values and no errors are thrown.

Review checklist

Reminder for reviewers

Signed-off-by: Kenneth Marut <kenneth.marut@ericsson.com>
@kenneth-marut-work kenneth-marut-work changed the title WIP: Bugfix/Stringify Task ENUMs for Presentation and PanelKind Bugfix/Stringify Task ENUMs for Presentation and PanelKind Jun 8, 2020
@akosyakov akosyakov added the tasks issues related to the task system label Jun 9, 2020
@marcdumais-work
Copy link
Contributor

@elaihau do you have time to review this PR?

Copy link
Contributor

@elaihau elaihau left a comment

Choose a reason for hiding this comment

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

I tested this change in my local environment. It worked properly. Thank you for the work !

@marcdumais-work
Copy link
Contributor

@akosyakov fine to merge?

Copy link
Contributor

@RomanNikitenko RomanNikitenko left a comment

Choose a reason for hiding this comment

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

I see that the corresponding API objects use different values. VS Code has other values as well.
So we should test some Theia plugin or VS Code extension which uses these objects to avoid regression from plugin side.

I can check this use case today, sorry for the delay...

@RomanNikitenko
Copy link
Contributor

I tried to test the use case described above, but faced with the related issue for master branch #8047

So, it's OK for me to merge the PR as is, we can come back and check the use case after resolving #8047

@marcdumais-work
Copy link
Contributor

@elaihau Ready to merge I think?

@elaihau
Copy link
Contributor

elaihau commented Jun 22, 2020

@elaihau Ready to merge I think?

@marcdumais-work Yes. I don't see objections from Roman either.

@marcdumais-work
Copy link
Contributor

Yes. I don't see objections from Roman either.

Please do the honours, Liang

@elaihau
Copy link
Contributor

elaihau commented Jun 22, 2020

Please do the honours, Liang

Oui chef

@elaihau elaihau merged commit c7182b5 into eclipse-theia:master Jun 22, 2020
@marcdumais-work
Copy link
Contributor

Oui chef

👍

Peek 2020-06-22 19-13

@kenneth-marut-work
Copy link
Contributor Author

thanks for the review + merge ✌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tasks issues related to the task system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants