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

Adding support for TaskProvider.resolveTask #71027

Merged
merged 6 commits into from Jul 3, 2019

Conversation

@joelday
Copy link
Contributor

commented Mar 23, 2019

First pass on #33523 for early review. Will be working on tests, possibly updating the example extension. Let me know if you have any suggestions for that.

Thanks @dbaeumer @alexr00!

@joelday joelday changed the title Adding support for TaskProvider.resolveTask. #33523 Adding support for TaskProvider.resolveTask Mar 23, 2019

@joelday

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2019

Wasn't able to repro the macOS integration test failure.

@alexr00

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

It looks like you are going in a good direction! I only took a quick look today. I will give a more thorough reveiw later in the week.

@joelday

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

@alexr00 Do you have any suggestions on which tests/test areas I should add coverage for this with? Thanks!

@alexr00 alexr00 added this to the April 2019 milestone Apr 3, 2019

@joelday

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

@alexr00 Just an assurance: I'm totally happy to rework this if the acceptance criteria needs to change. Also happy to do some general refactoring if this currently contributes to any debt.

@alexr00

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

@joelday, I'm sorry it took me so long to take a look at your PR! Most of it looks good. However, I think you are calling provider.resolveTask in the wrong place. resolveTask is intended to be used to increase task performance.
For example, users probably are most interested in the tasks defined in tasks.json. Calling provideTasks for every task provider can be slow. Instead of calling provideTasks for every provider, we can first call resolveTask for all the tasks in tasks.json and return those first to show in the quick pick.
Or, a simpler example: you can use a custom keybinding to run a specific task. Instead of getting all tasks by calling provideTasks on every provider then finding the task that the user wants, we can just call resolveTask and quickly get the one task they asked for.
So the pattern would be: do we already know which task the user wants? Call resolveTask. Otherwise, get all tasks by calling provideTasks for every provider then let the user pick the task.

@alexr00
Copy link
Member

left a comment

See my long comment about where resolveTask should be called.

@joelday

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

@alexr00 I had a feeling that these requirements may have not been 100% right: #33523 (comment)

This should be an easy change and I don't think it'll conflict. I'll need to do some refactoring, largely along how things are asynchronously enumerated/transformed/joined. That function has a lot of complexity.

Thank you!

@joelday

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

@alexr00 For the specific task run scenario: As I dig into the implications for the various current callers of getGroupedTasks, this gets pretty complex, bleeding into createRunnableTask and its resolution/disambiguation logic, which is currently written against already resolved tasks. Will have to think about it a bit, but I'm unsure when I'll be able to revisit it.

As for the Quick Pick scenario, I'm unsure of the performance benefit if provideTasks still has to be called on each provider either way. Additionally, the task providers don't have any way of knowing if a task previously provided via resolveTask should be excluded from the subsequent call to provideTasks. Right now, resolveTask is only called for tasks that weren't contributed, so there's no de-duplication problem. If resolveTask isn't used for Quick Pick at all, then only contributed tasks will show up, as is the current behavior.

@alexr00

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

I agree that this is not a trivial change. There's no rush to get it done though.

For cases like the VS Code repo the quick pick scenario could provide a significant improvement. 9.9 times out of 10 I want to run one of the tasks in the tasks.json. However, I have to wait up to a minute for all of our gulp and typescript tasks to enumerate (gulp in particular is slow to enumerate).

@joelday

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

@alexr00 absolutely agreed on the impact of the performance for quick pick. Is it possible to present items in a quick pick incrementally? Otherwise I'm not sure how to reconcile presenting both the quickly resolved tasks without still having to wait for the slow contributed ones to finish first.

@alexr00 alexr00 modified the milestones: April 2019, May 2019 May 9, 2019

@alexr00 alexr00 modified the milestones: May 2019, June 2019 May 31, 2019

@alexr00 alexr00 modified the milestones: June 2019, Backlog Jul 1, 2019

@joelday

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

@alexr00 @dbaeumer Just pinging on this. My impression is that this was really a disconnect on the intended design of the Tasks feature entirely, so I'm happy to just decline this PR.

I do have some (hopefully useful) feedback on my experience contributing here, especially in contrast to having worked with @jrieken after I proposed and implemented the insertSnippetAPI.

  1. Based on #33523 (comment), I think what the OP was asking for and expected resolveTask to do was pretty clear, but it was definitely missed.

  2. I sought and received confirmation of my implementation before proceeding: #33523 (comment). That approach pretty clearly did not satisfy what is now expected.

Should resolveTask only ever be called to speed up the start time and validation of tasks? In my experience, the largest wait is when using quick pick, but this idea seems incompatible with that both from a technical and UX standpoint.

Thanks!

@alexr00

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

@joelday I'm sorry I've let this PR lapse. Looking at it again with a fresh set of eyes, I think that it resolveTask is done in the right place. The speed up from the quick pick wouldn't be from adding tasks to it slowly (as you pointed out this isn't an option right now), but from turning off task auto detection for a task type in settings (ex: "gulp.autoDetect": "off",) and allowing the extension to instead only provide tasks for for those that are customized in tasks.json. The advice you got in #33523 (comment) from the original API writer stands. The misunderstanding was mine.

As you say, what #33523 (comment) asks for isn't what resolveTask is intended to do. The issue for the kind of customization that OP wanted is #58836. I'll make some notes in the issue that make that clear.

Having an API that doesn't do anything (resolveTask) is confusing. I would like to get it implemented and properly documented. I'll take another thorough look at your PR today and see if I have any feedback.

resolveTask: (task: ConfiguringTask) => {
const dto = TaskDTO.from(task);

// Only named tasks can be resolved. (That should be the case, right?)

This comment has been minimized.

Copy link
@alexr00

alexr00 Jul 3, 2019

Member

As far as I can see, not having a name is fine. The important thing is that the task has a definition so that the task provider knows what to do with it.

try {
const resolvedTask = await provider.resolveTask(configuringTask);
if (resolvedTask) {
result.add(key, resolvedTask);

This comment has been minimized.

Copy link
@alexr00

alexr00 Jul 3, 2019

Member

This adds the task as a contributed task in the quick pick. We need to add it as a configuring task.

@dbaeumer

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

@joelday & @alexr00 thanks a lot for working on this. Highly appreciated.

@alexr00

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

@joelday this works very nicely! I made two very small changes in the areas I left comments while I was testing it. I also selfishly updated our builtin gulp extension to use resolveTask. I always disable gulp task auto detection in my settings, which means in the vscode repo I always get errors when I run tasks since they are in the tasks.json. No more errors!

@alexr00

alexr00 approved these changes Jul 3, 2019

@alexr00 alexr00 merged commit b6089b1 into microsoft:master Jul 3, 2019

5 checks passed

VS Code Build #20190703.57 succeeded
Details
VS Code (Linux) Linux succeeded
Details
VS Code (Windows) Windows succeeded
Details
VS Code (macOS) macOS succeeded
Details
license/cla All CLA requirements met.
Details

@alexr00 alexr00 modified the milestones: Backlog, July 2019 Jul 3, 2019

@joelday

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

Thanks! Regarding what #58836 asks for, this does allow an extension developer to "abuse" the API for that purpose, so that would be my only concern at this point.

@joelday

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

Also, please don't take my follow-up to imply impatience. Having a PR to review doesn't change prioritization!

@alexr00

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

I am working on debt items this week, and an unimplemented API looked like debt to me. You had good timing :)

Yes, "abuse" is possible. It might be worth adding a check on the task after we get it back from resolveTask to make sure that it's essential/identifying properties are unchanged.

@joelday joelday deleted the joelday:task-provider-resolve-task branch Jul 6, 2019

@joelday

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

@alexr00 Thanks again!

}

if (CustomExecutionDTO.is(resolvedTaskDTO.execution)) {
await this.addCustomExecution(taskDTO, <vscode.Task2>task);

This comment has been minimized.

Copy link
@GabeDeBacker

GabeDeBacker Jul 22, 2019

Contributor

@joelday & @alexr00 -> Joel, this is the wrong DTO and task. It should be resolvedTaskDTO. and resolvedTaskDTO

This comment has been minimized.

Copy link
@alexr00

alexr00 Jul 29, 2019

Member

If we allow the provider to change the DTO properties, then we have a different task since those properties are the fingerprint of the task.

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