Extracted the taskId criteria while searching the input #134
Conversation
roussak
changed the title
|
Thanks for your contribution. It looks sensible. Please update the changelog and run |
roussak
force-pushed
the
master
branch
2 times, most recently
from
5bfd24e to
952bbea
Compare
|
I know that the readme says that I would recommend:
Would you like to implement that in this PR or would you like me to do it separately? |
|
I think, this feature doesn't "fit" into this commit "by design" -- it's relates to slightly different theme. Furthermore, I work on C/C++, not TS. :) |
|
That is to say I should do it? I'm also a C++ developer professionally, and I don't use VSCode 😆 |
|
I implemented the first part in #136. |
So, what are you doing here then?.. :')
And what about this PR? Will you merge it later, after implementation the second part of your Great Plans? :) |
|
@roussak Yes, I am preparing to merge your PR. I'm trying to do it carefully to avoid breaking things for people, even if that use case is an abuse of |
|
@roussak Sorry for the delay. I finally had some time to work on this. Please rebase on master and fix the conflicts, then I'll review it. |
…#131 The very presence of an "Id" in the name suggests that this entity must be something unique. But, previously the "taskId" was used among of the command and its arguments while searching the input object in the resolveTaskToInput() function. Among of other effects, this was leading to exception throwing for the use-case like described in the ticket (combined usage of the Shell Command and CommandVariable extensions usage). This patch if doesn't fix this issue, then gives a workaround for it. Specifying the "taskId" for the Shell Command calls lets the variables substitution with CommandVariable extension (using the `variableSubstArgs` argument).
| ## [1.16.1] 2025-02-04 | ||
|
|
||
| - Extracted the `taskId` criteria while searching the input. (#131) | ||
|
|
||
| ## [Unreleased] xxxx-xx-xx |
There was a problem hiding this comment.
Please delete this line and update the date for 1.16.1
| @@ -466,8 +466,14 @@ export class CommandHandler { | |||
| duplicateTaskIds.add(input.args.taskId); | |||
| } | |||
|
|
|||
| if (input.args.command === this.command && | |||
| input?.args?.taskId === taskId && | |||
| // FIXME: Aside of the buisness-logic of the program, | |||
There was a problem hiding this comment.
This should be fine, shouldn't it?
There was a problem hiding this comment.
You added a FIXME comment, but I don't see the problem with assigning result multiple times.
| @@ -466,8 +466,14 @@ export class CommandHandler { | |||
| duplicateTaskIds.add(input.args.taskId); | |||
| } | |||
|
|
|||
| if (input.args.command === this.command && | |||
| input?.args?.taskId === taskId && | |||
| // FIXME: Aside of the buisness-logic of the program, | |||
There was a problem hiding this comment.
I forgot to update the comment on 463. Could you please do that?
There was a problem hiding this comment.
How?
By the way, the comment on line 463 says that we return the first match, but technically we return the last match. Of course, when we are searching by Id, the match most probably will be the only (i.e. will hide the potential problem mentioned in my FIXME).
There was a problem hiding this comment.
Actually, I'd be fine with just deleting the comment. I added it because I think not everyone is familiar with generators, but now it's just an array.
We can get multiple matches if getSectionInputs("tasks", folder); (multi-workspace aware) returns the same as yield* getSectionInputs("tasks"); This is expected and fine.
|
It would be great if you could add a test case using commandvariable |
For sure, but is there any guideline (example) for this? |
The current testing situation isn't ideal (mostly because I haven't found a good way to test VSCode). You can look into |
|
I see. But, probably, I will be able to do this only next week. |
The very presence of an "Id" in the name suggests that this entity must be something unique. But, previously the "taskId" was used among of the command and its arguments while searching the input object in the resolveTaskToInput() function. Among of other effects, this was leading to exception throwing for the use-case like described in the ticket (combined usage of the Shell Command and CommandVariable extensions usage). This patch if doesn't fix #131, then gives a workaround for it. Specifying the "taskId" for the Shell Command calls lets the variables substitution with CommandVariable extension (using the
variableSubstArgsargument).