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.

Sign up for GitHub

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

Jump to bottom

Fix converting of task with custom type #5591

Merged
merged 1 commit into from
Jul 4, 2019
+60 −15
Merged

Fix converting of task with custom type #5591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 58 additions & 14 deletions packages/plugin-ext/src/plugin/type-converters.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,16 +169,18 @@ describe('Type converters:', () => {
});

describe('convert tasks:', () => {
const type = 'shell';
const customType = 'custom';
const shellType = 'shell';
const label = 'yarn build';
const source = 'source';
const command = 'yarn';
const commandLine = 'yarn run build';
const args = ['run', 'build'];
const cwd = '/projects/theia';
const additionalProperty = 'some property';

const shellTaskDto: ProcessTaskDto = {
type,
type: shellType,
label,
source,
scope: undefined,
Expand All @@ -192,7 +194,7 @@ describe('Type converters:', () => {
name: label,
source,
definition: {
type,
type: shellType,
additionalProperty
},
execution: {
Expand All @@ -204,24 +206,39 @@ describe('Type converters:', () => {
}
};

const taskDtoWithCommandLine: ProcessTaskDto = {
type,
label,
const pluginTaskWithCommandLine: theia.Task = {
name: label,
source,
scope: undefined,
command,
args,
options: { cwd }
definition: {
type: shellType,
additionalProperty
},
execution: {
commandLine,
options: {
cwd
}
}
};

const pluginTaskWithCommandLine: theia.Task = {
const customTaskDto: ProcessTaskDto = { ...shellTaskDto, type: customType };

const customPluginTask: theia.Task = {
...shellPluginTask, definition: {
type: customType,
additionalProperty
}
};

const customPluginTaskWithCommandLine: theia.Task = {
name: label,
source,
definition: {
type
type: customType,
additionalProperty
},
execution: {
commandLine: 'yarn run build',
commandLine,
options: {
cwd
}
Expand Down Expand Up @@ -252,7 +269,34 @@ describe('Type converters:', () => {

// then
assert.notEqual(result, undefined);
assert.deepEqual(result, taskDtoWithCommandLine);
assert.deepEqual(result, shellTaskDto);
});

it('should convert task with custom type to dto', () => {
// when
const result: TaskDto | undefined = Converter.fromTask(customPluginTask);

// then
assert.notEqual(result, undefined);
assert.deepEqual(result, customTaskDto);
});

it('should convert task with custom type from dto', () => {
// when
const result: theia.Task = Converter.toTask(customTaskDto);

// then
assert.notEqual(result, undefined);
assert.deepEqual(result, customPluginTask);
});

it('should convert to task dto from custom task with commandline', () => {
// when
const result: TaskDto | undefined = Converter.fromTask(customPluginTaskWithCommandLine);

// then
assert.notEqual(result, undefined);
assert.deepEqual(result, customTaskDto);
});
});

Expand Down
3 changes: 2 additions & 1 deletion packages/plugin-ext/src/plugin/type-converters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,7 +651,8 @@ export function toTask(taskDto: TaskDto): theia.Task {
result.execution = getProcessExecution(taskDto as ProcessTaskDto);
Copy link
Contributor

Choose a reason for hiding this comment

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

just wondering: do we need to use types.ProcessExecution.is(execution) in the if-statement in line 650, since you added a "customType" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you, I'll review it today

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elaihau I updated the PR, please review.
about your question - we use types.ProcessExecution.is(execution) when we do converting from theia.Task to TaskDto, but we don't need the same when we converting from TaskDto to theia.Task. For this case we use check by type only.
It's because at converting theia.Task of 'process' type 'process' filed is converted to 'command' filed of TaskDto. ProcessTaskRunner recognizes task by type and expects command field at the moment.

}

if (taskType === 'shell') {
const execution = { command, args, options };
if (taskType === 'shell' || types.ShellExecution.is(execution)) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder whether we need similar check for taskType === 'process' above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think - no, the reasons I wrote here. But let me know if I should to reconsider that.

Copy link
Member

Choose a reason for hiding this comment

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

ok, please merge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, thanks for testing changes and review!

result.execution = getShellExecution(taskDto as ProcessTaskDto);
}

Expand Down