[vscode] ShellExecution update with undefined command

[rschnekenbu]

What it does

Adds in the API the possibility to have an undefined command or args for the shellexecution. This was already supported by the implementation but not clearly defined in the API.

How to test

1. Install the following extension:

• src: vscode-math-evaluator-extension-0.0.1-src.zip
• zipped vsix: vscode-math-evaluator-extension-0.0.1.zip

2. Open a new markdown file and add this line to it: 1+1.
3. The extension will create 2 tasks with 2 different shell execution, 1 with a command line (and then command and args undefined), the second with the command+arg, and then commandLine undefined.
4. Change active editor to refresh the tasks if necessary (the tasks are recomputed on active editor change)
5. 2 messages will be displayed, one for each tasks describing the content of the shell execution for each task.
6. Ensure that the command+ args or commandLine are undefined given the task
7. Tasks can be run to check if ShellExecution handles nicely the 2 cases using run task... command => Math => the task you want to check.

issue14940.mp4

Follow-ups

No follow ups

Breaking changes

• This PR introduces breaking changes and requires careful review. If yes, the breaking changes section in the changelog has been updated.

Attribution

Contributed on behalf of STMicroelectronics

Review checklist

• As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• As a reviewer, I agree to behave in accordance with the review guidelines

[tsmaeder]

These are just drive-by fixes, right?

[rschnekenbu]

Yes, the comments were changed in equivalent file in vscode, but these are only better explanation on the API behavior.

[tsmaeder]

If I read this change correctly, it does not really change the usage of the class in any way, right? it probably would force literals to include a value for "command" if it was an interface, but not for a class.
That said, if I look at the implementation of ShellExecution in types-impl.ts, I see this:

set commandLine(value: string) {
        if (typeof value !== 'string') {
            throw illegalArgument('commandLine');
        }

So it throws an exception if I assign undefined. But that's not consistent with the typing. What's the reason?

[rschnekenbu]

@tsmaeder, basically, for the shell execution, we are supposed to have either commandLine set to a valid string, or the command and eventually args.
The check in the types-impl is redundant with the one in the constructor. I would also not expect the shell execution to be changed at runtime. In case we still want to change at runtime, this may be done using an atomic change between command line and command + args, to ensure only one group is defined at a time.

[tsmaeder]

I have a command value, shouldn't we be checking that we don't have a commandLine value? Because we're not supposed to have both, right? Anyway, why aren't we respecting the interface and complaining at execution time that the user set something unusable.

I would also not expect the shell execution to be changed at runtime.

But the API allows it, so people are going to do it 🤷 This check might break exensions that do

execution.command  = undefined;
execution.commandLine = 'ls -ls`;

which would work perfectly fine otherwise.

[rschnekenbu]

So if I understand you, we should allow undefined in the setCommand, setCommandLine (and setArgs), remove the checks done on the values, and allow the getters to return also undefined in types-impl?
Otherwise, we would have issues with the first line
execution.command = undefined;
both commandLine and command would be undefined at the same time.

[tsmaeder]

The typing says you can assign undefined to command, so if the implementation is not allowing that, there needs to be a reasons for that. If we try to not allow both command and commandLine, we would have to have a different check. It seems the checks are coming from the extHostTypes.ts in VS Code. But those checks would still allow for both command and commandLine to be set. So we're bug-compatible with VS Code in this respect, but it is still wrong and it still bugs me 🤷

[tsmaeder]

This is a typing-only change that should not affect clients and the implementation, albeit bogus, is compatible with VS Code and has been like that for years.

[rschnekenbu]

Thanks @tsmaeder, I created #15062 as a follow up to tackle the implementation issues and not this API update.