theia icon indicating copy to clipboard operation
theia copied to clipboard

Fix filtering of some auto-detected and customized tasks

Open bd82 opened this issue 3 years ago • 5 comments

fixes #9494

What it does

Resolves issue: https://github.com/eclipse-theia/theia/issues/9494

How to test

A repo reproducing the issue is provided here

  • https://github.com/bd82/theia_tasks_filter_bug

Review checklist

Reminder for reviewers

bd82 avatar May 20 '21 11:05 bd82

Status Update

I don't think I can fully resolve this issue. I can contribute with:

  • Identifying the issue and provide a reproducible scenario
  • Identifying things that seem like possible logic bugs in the existing code and raise them for discussion.

But It seems much more in-depth knowledge of the Tasks in Theia and their implementation (7,000~ LOC...) is needed to actually resolve this.

From my limited interaction with the task package. I get the feeling something here is far too complex. I don't know if this is due to any, all, or none of the possible causes below:

  • Legacy behaivor.
  • Supporting both VSCode tasks and Theia Tasks.
  • Patches on Patches.
  • lack of tests.
  • Some inherent design issues.

Some things I do recognize are:

  • There are almost 7,000 LOC of code in the task src directory, only ~300 of which are tests.
    • Which seems like a-lot of code.
  • There are 49 open issues and 117 closed issues labeled under tasks.
    • Which seem like a-lot of related issues.

If this was a project I "owned" (and knew in-depth) I would try to evaluate the reasons for the complexity and the feasibility of a re-write. But that is far beyond the scope of my capabilities here...

And perhaps my gut feeling is completely wrong and all the complexity here is fully inherent to the problem space (no accidental complexity...).

So to summarize:

Is there anyone from Theia who understands this area deeply and can attempt to resolve the original issue https://github.com/eclipse-theia/theia/issues/9494 ?

bd82 avatar Jun 10 '21 10:06 bd82

@bd82 thanks a lot for working on this. The tasks system in Theia is quite the beast that suffers (IMO) from getting some of the basic concepts wrong and then working around that (and don't get me started on the naming of things ;-)). Simplifying things (like your PR does) is a step in the right direction. Do not get discouraged!

tsmaeder avatar Jun 10 '21 11:06 tsmaeder

  • There are almost 7,000 LOC of code in the task src directory, only ~300 of which are tests.

    • Which seems like a-lot of code.

I completely agree with this sentiment, and if you wanted to shift the focus of this PR from implementing a particular solution to increasing test coverage, I would support that. That would quickly place you among the most knowledgeable developers in this area and almost certainly expose a number of oddities and conflicting assumptions that would provide good starting points for later fixes and simplifications.

colin-grant-work avatar Jun 10 '21 14:06 colin-grant-work

I completely agree with this sentiment, and if you wanted to shift the focus of this PR from implementing a particular solution to increasing test coverage, I would support that.

I am trying to solve the original bug as it affects a business scenario for the organization I work for...

Simplifying things (like your PR does) is a step in the right direction. Do not get discouraged!

I have limited time before I go on a long summer vacation. I will attempt to investigate the label question before I leave for vacation, as I think that is the main open question in this PR

  • Is it mandatory?
    • In Theia.
    • In VSCode.
  • Could the deleted code in this PR have added a "missing" label in some scenarios?

bd82 avatar Jun 20 '21 11:06 bd82

Okay what I've learned is:

  • This label property is used extensively in VSCode as well.
  • It is not part of any of the tasks related APIs I can find on VSCode official API docs.
  • It seems to be a computed property.
  • I cannot seem to find where in fact it is computed though.

Currently this makes me think that getTasks() should perhaps TaskCustomization[] instead of TaskConfiguration[]. But this depends on what happens to plain tasks written in the tasks.json Do they undergo some transformation and have a label property added to them in the internal in memory representation or is getTasks() expected to return those tasks.json tasks "as is" without any changes.

But I need to investigate it farther, but that won't happen anytime soon as I will be OOO for most of the summer... 😄

bd82 avatar Jul 01 '21 12:07 bd82

@tsmaeder : Could you please decide on what to do with this PR?

JonasHelming avatar Nov 15 '22 08:11 JonasHelming

Since this would need some work which no-one can do right now, let's close the PR.

tsmaeder avatar Nov 21 '22 10:11 tsmaeder