feat: implement configuration to change sub command for test, bench and doctest

[dfireBird]

fixes #21137

[ChayimFriedman2]

LGTM, minus one thing: we generally have X_overrideCommand called for completely overriding the command, and be Option<Vec<String>>, while "X_command": "foo" is a single String and is being integrated in the existing place (cargo foo --args). You can have both are either, but please don't confuse them.

[dfireBird]

Oh should this be then Option<Vec<String>>? Cuz we can't have single Single string here especially for doc tests.

[ChayimFriedman2]

As I said, we generally have two configs, one X_overrideCommand: Option<Vec<String>> and one X_command: String. We can not have one if needed.

[dfireBird]

I'm really sorry, I’m having some difficulty understanding what you mean.
Are you saying we need to have X_command: String and X_overrideCommand: Option<Vec<String>>, but we should not only have X_overrideCommand as I currently do now, is that right?

[ChayimFriedman2]

Yes.

[dfireBird]

Oh I see. Thanks. Then I'll make the change for override command to be command and include an arguments config like so:

rust-analyzer.runnables.test.command: String = "test"
rust-analyzer.runnables.test.args: Vec<String> = []

So for example to use nextest those two will be

{
  "rust-analyzer.runnables.test.command": "nextest",
  "rust-analyzer.runnables.test.args": ["run"]
}

Similar for bench and doctest as well.

[dfireBird]

I've pushed the changes as separate commit, so that if this change is fine I'll squash them into one before merging (if that's fine).

P.S: I'm sorry for earlier. I had some difficultly understanding what you changed were asked of me.

[ChayimFriedman2]

That's not what I suggested. I suggested a pair of fields for each configuration, X_command and X_overrideCommand. No X_args. If this setup does not work for doctests, we cam not provide X_command for it.

[dfireBird]

Ah I see. Ok, then I'll remove doctests and include only bench and test.

[ChayimFriedman2]

That's still not how we handle those settings.

overrideCommand should not be passed as arguments; if overrideCommand is set, it fully determines the command to run. That's why it's called "override".

Also, you forgot the doctests overrideCommand.

[dfireBird]

overrideCommand should not be passed as arguments; if overrideCommand is set, it fully determines the command to run. That's why it's called "override".

So the overrideCommand might not even need to invoke cargo (i.e we should not add the overrideCommand to cargo_args)? If that's the case, we lose the ability to add flags or options related to features and target kind or is that fine?

Also, you forgot the doctests overrideCommand.

It's fine if I just overrideCommand for doctests? Without command

[ChayimFriedman2]

So the overrideCommand might not even need to invoke cargo (i.e we should not add the overrideCommand to cargo_args)?

True.

If that's the case, we lose the ability to add flags or options related to features and target kind or is that fine?

You can still use overrideCommand, but you'll have to type the full command.

It's fine if I just overrideCommand for doctests? Without command

Yes, I think.

[dfireBird]

I'm fine with changing it s it follows as our existing guidelines of configurations. But I feel like it's kinda doesn't work correctly for multiple crate projects like ours, since r-a then won't add the args like --package on overrideCommand

Also the executable args remains as is for the overrideCommand as well right? That is if overrideCommand is used, we still populate the exectable args and pass it or will that have to be not populated as well?

EDIT:
We will have to change the kind in lsp::ext::Runnable as Shell right then, if we are override the full command?

[ChayimFriedman2]

Hmm, we can have placeholders that are substituted for the package name, e.g. ${package}.

[dfireBird]

Hmm, Okay. This will complicate it quite a bit, I'll go back a step and come up with better solution of how to use config instead of the plain stuff I do now.

[ChayimFriedman2]

Just two things, then LGTM.

[ChayimFriedman2]

Thanks!