Extracted the taskId criteria while searching the input

[roussak]

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 variableSubstArgs argument).

[MarcelRobitaille]

Thanks for your contribution. It looks sensible. Please update the changelog and run npm install to update the version also in the package-lock.json

[MarcelRobitaille]

I know that the readme says that taskId is unique, but I know of people abusing this to share remembered data between tasks. This is very powerful when combined with ${taskId:...}.

I would recommend:

• Showing a warning if a taskId is duplicated
• Adding a field rememberUnder to specify an alternative taskId under which to remember the selection. Everyone using the taskId hack will have to migrate to this.

Would you like to implement that in this PR or would you like me to do it separately?

[roussak]

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. :)

[MarcelRobitaille]

That is to say I should do it?

I'm also a C++ developer professionally, and I don't use VSCode :laughing:

[MarcelRobitaille]

I implemented the first part in #136.

[roussak]

I'm also a C++ developer professionally, and I don't use VSCode 😆

So, what are you doing here then?.. :')

I implemented the first part in #136.

And what about this PR? Will you merge it later, after implementation the second part of your Great Plans? :)

[MarcelRobitaille]

@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 taskId. I hope I can proceed soon. I'm sorry for the delay.

[MarcelRobitaille]

@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.

[MarcelRobitaille]

It would be great if you could add a test case using commandvariable

[roussak]

It would be great if you could add a test case using commandvariable

For sure, but is there any guideline (example) for this?

[MarcelRobitaille]

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 src/lib/CommandHandler.test.ts for examples. You can put your tasks.json in src/test/testData/.... You also have to generate mockData.ts which is a bit annoying. You can look into the comments at the top of src/mocks/vscode.ts for this.

[roussak]

I see. But, probably, I will be able to do this only next week.

[roussak]

You also have to generate mockData.ts which is a bit annoying. You can look into the comments at the top of src/mocks/vscode.ts for this.

I don't understand how to do that… :( If I substitute the import with the '../mocks/vscode', errors are being appeared. As well, it's not clear to me where and when should I call the vscode.Spy.write().

[MarcelRobitaille]

Sorry, I know it's really hacky and not intuitive.

1. In CommandHandler.ts, change import * as vscode from "vscode"; with import * as vscode from "../mocks/vscode". Typescript will start complaining a lot. Just ignore it for now.
2. In the CommandHandler constructor, put vscode.Spy.write();. Put a breakpoint on this line.
3. Start debugging. It will warn that there are "errors". It's only TypeScript complaining. Press "Debug anyway".
4. In the new window that's running the extension, open the folder where you want to put your test data (src/test/testData/example/). Run a task that uses this extension. You should arrive to your breakpoint.
5. When you step over vscode.Spy.write();, you should see data in the Debug Console. Copy this and save it into mockData.ts.
6. Add export default at the start and change all paths to where you checked out the repo to ${__dirname} (check another mockData.ts for an example)

If this is too much, we can merge it without tests and I can do them myself.

[roussak]

I tried this manual, but as far as I can see the calls object should be non-empty in order to perform the testing. So, it'll probably better if you'll do this point. I will rebase the commit shortly.

[MarcelRobitaille]

@roussak thanks for your contributions