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

Use correct resolved TDO and task for custom execution #77759

Merged
merged 4 commits into from Jul 30, 2019

Conversation

@GabeDeBacker
Copy link
Contributor

commented Jul 22, 2019

This address issue #77757.

When using custom executions in conjunction with resolved tasks, the wrong variables where being passed to addCustomExecution and addCustomExecution2. They needed to be passed the resolvedDTO and the resolvedTask.

This change was verified by ensuing that task resolution works correctly after modifying a default build task in tasks.json for our internal MS extension. Both "terminal renderer" and "virtual terminal process" paths were verified.

@GabeDeBacker

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

@alexr00 and @joelday, can you please take a look?

Thanks!

@Tyriar Tyriar added this to the July 2019 milestone Jul 23, 2019

@Tyriar

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

@alexr00 please consider for July when you get back

@alexr00

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

I'm not sure this is a good path to go down. It is intentional that we don't allow changes to a task's provider provided properties. Those properties function as an ID of the task if they are changed, it is a different task.

@GabeDeBacker

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

I think I’m confused on what resolveTask is supposed to do (hence issue #77759).

What we can do in our resolveTask is essentially return as “new task” as long as the task information passed to “resolve task” is something we can execute.

Is the intention of “resovleTask” to resolve it to something that the provider provided during “provideTasks” (i.e. an already existing task?).

I guess what I’m super confused about is should the user EVER be able to edit tasks.json? If they do, then how would a task there ID ever match what is returned through “provideTasks”?

Our task provider can actually resolve to a new task with no problems as long as the edits to task.json match our schema, which VSCode happily guides the user through if your task definition information is correct in the package.json for the extension 😊

Please help me understand 😊

@alexr00

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

tasks.json is for modifying non-provider specific task properties. If a task provider defines a property for their task in their package.json, then those properties are immutable for that task. We use them as an ID of the task so it needs to be consistent. Things like background, presentation, and group are all fair game to be modified in tasks.json.

This confusion over what resolveTask does has happened repeatedly. I will update the documentation.

@GabeDeBacker

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

Spoke with Alex about this offline. The goal here is to make sure that the resolved task definition is not different than the original task definition. If they are the same, then the resolved task can be used.

@alexr00
Copy link
Member

left a comment

We talked about this some more, and the goal of this change isn't to change the task definition (which is dangerous because it changes the task's ID). All that's needed in addition what's already here is a check that the task definition hasn't changed.

@Tyriar

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Merging as only install deps on mac failed

@Tyriar Tyriar merged commit 82f97f7 into microsoft:master Jul 30, 2019

3 of 5 checks passed

VS Code Build #20190730.40 failed
Details
VS Code (macOS) macOS failed
Details
VS Code (Linux) Linux succeeded
Details
VS Code (Windows) Windows succeeded
Details
license/cla All CLA requirements met.
@alexr00

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Thank you for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.