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

Add support for TaskGroup.id and isDefault. #56

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tsmaeder
Copy link
Collaborator

What it does

Adds support for id and isDefault and includes a small drive-by fix in the Task implementation where the taskRunOptions object was not properly initialized.

Fixes eclipse-theia#11519

Contributed on behalf of ST Microelectronics

How to test

  1. Install the attached vscode extension. It is built from https://github.com/tsmaeder/castletest
  2. Open a workspace that generates some tasks. I use the source of Theia
  3. Configure some tasks, playing with the "id" and "default" fields
  4. Inspect the task by invoking "Dump task info to console" from the command palette and make sure the right values are shown for "isDefault" and "id".

Review checklist

Reminder for reviewers

@tsmaeder
Copy link
Collaborator Author

castletest-0.0.1.zip

@sgraband sgraband self-requested a review December 1, 2022 11:25
@sgraband
Copy link

sgraband commented Dec 1, 2022

Thank you for the changes! Looks good to me. Just one minor nitpick/question:

Why do we have two terms for the same field (kind and id)? Is this to achieve compatability with vscode tasks or could we align this?

@tsmaeder
Copy link
Collaborator Author

tsmaeder commented Dec 1, 2022

Why do we have two terms for the same field (kind and id)?

I'm glad you asked ;-) Because tasks are a mess: their typing is a mix of the schema of the tasks.json file, our internal structures and what the VS Code API wants. So task kind and task id are NOT really the same thing: they might be the id of a TaskGroup object from the VSCode API or they might be a string that is written in a tasks.json (where only "build" and "test" are allowed, by the way). Calling it id would just paper over the lack of clarity there. Different things should be name differently!

@@ -1443,6 +1443,7 @@ export interface CommandProperties {
};
}

export type TaskGroupKind = 'build' | 'test' | 'rebuild' | 'clean'
Copy link

Choose a reason for hiding this comment

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

Suggested change
export type TaskGroupKind = 'build' | 'test' | 'rebuild' | 'clean'
export type TaskGroupKind = 'build' | 'test' | 'rebuild' | 'clean';

Seems like the linter requires a semicolon here.

Copy link

@sgraband sgraband left a comment

Choose a reason for hiding this comment

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

Thanks for the clarification. The linter and the tests are complaining, though. Could you check that?

@tsmaeder
Copy link
Collaborator Author

tsmaeder commented Dec 2, 2022

The tests complain that that "Values have same structure but are not reference-equal". I suspect it's because the TaskGroup constants use the unwrapped TaskGroup constructor, whereas type-converter.ts uses the @Es5Compat-annotated version of the constructor. I'm not sure whether that is a real problem or not, you're probably a better judge of it (seeing that you did eclipse-theia#9436). I can fix the test, though 🤷

Copy link

@sgraband sgraband left a comment

Choose a reason for hiding this comment

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

Great thank you! Using the es5-wrapped constructor should not be an issue. LGTM 👍

Adds support for id and isDefault and includes a small drive-by fix in
the Task implementation where the taskRunOptions object was not properly
initialized.

Contributed on behalf of ST Microelectronics

Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
ndoschek added a commit that referenced this pull request Jul 30, 2024
ndoschek added a commit that referenced this pull request Jul 30, 2024
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.

[vscode] TaskGroup misses support for id and isDefault
2 participants