fixes #8975 The literal string "<leader>" is substituted for the leader key when repeating a command-line command

[HenryTSZ]

What this PR does / why we need it:

fixes #8975 The literal string "" is substituted for the leader key when repeating a command-line command

Which issue(s) this PR fixes

#8975

Special notes for your reviewer:

The commandList here saves the ex command, which typically do not include leader.

Vim/src/state/recordedState.ts

Lines 58 to 59 in 5365305

	// Used for the registers and macros that only record on commandList
	result = this.commandList.join('');

Therefore, we do not need the following replace

Vim/src/state/recordedState.ts

Lines 62 to 64 in 5365305

	return result
	.replace(new RegExp(configuration.leader.replace(ESCAPE_REGEX, '\\$&'), 'g'), '<leader>')
	.replace(BUFFERED_KEYS_REGEX, '');

[J-Fields]

Can you please write a test case for this?

[HenryTSZ]

Can you please write a test case for this?

I don't know how to write test cases. Do you have any documentation for reference?

[mvandenhoek]

I don't know how to write test cases. Do you have any documentation for reference?

I think the test will be similar to this one:

Vim/test/macro.test.ts

Lines 25 to 30 in cfbd076

	newTest({
	title: 'Can repeat last invoked macro',
	start: ['|foo = 1', "bar = 'a'", 'foobar = foo + bar'],
	keysPressed: 'qaA;<Esc>Ivar <Esc>qj@aj@@',
	end: ['var foo = 1;', "var bar = 'a';", 'var| foobar = foo + bar;'],
	});

To test for my issue, you'll need two such testcases. One where leader is (space) and one where it is \ (backslash), and those keys then need to be used for a substitution command in keysPressed.

My guess is that you'll have to use the configuration to set the leader key before starting the test, see https://github.com/VSCodeVim/Vim/blob/master/src/configuration/configuration.ts

I'm not sure if for the test to simulate those key presses, the keysPressed will have to have the actual character e.g. \, or you have to write <leader>. Perhaps @J-Fields can shed some light on this?

Either way I would first check that it fails the test before your change, and then confirm that it passes after your change.

[J-Fields]

I'm not sure if for the test to simulate those key presses, the keysPressed will have to have the actual character e.g. \, or you have to write <leader>. Perhaps @J-Fields can shed some light on this?

I think you have to write <leader>, but could be misremembering

[HenryTSZ]

@mvandenhoek @J-Fields
Hi, Sorry, I've been a bit busy these past few days

After restoring the code

Vim/src/state/recordedState.ts

Lines 58 to 59 in 5365305

	// Used for the registers and macros that only record on commandList
	result = this.commandList.join('');

and adding the following test code

  suite('`:` (command) register used as macro and command with leader key', () => {
    setup(async () => {
      const configuration = new testConfiguration.Configuration();

      // for testing with <leader>
      configuration.leader = 'o';

      await setupWorkspace(configuration);
    });

    newTest({
      title: 'Repeat :s and command with leader key',
      start: ['|old', 'old', 'old'],
      keysPressed: ':s/old/new\nj@:j@@',
      end: ['new', 'new', '|new'],
    });

    newTest({
      title: 'Repeat :s and command with leader key',
      start: ['|old', 'old', 'old'],
      keysPressed: ':s/old/new\nj@:j@@',
      end: ['new', 'old', '|old'],
      statusBar: 'E486: Pattern not found: <leader>ld',
    });
  });

The running result is as follows

    `:` (command) register used as macro and command with leader key
      1) Repeat :s and command with leader key
      ✔ Repeat :s and command with leader key (40ms)

  1 failing
  1) Record and execute a macro
       `:` (command) register used as macro and command with leader key
         Repeat :s and command with leader key:

      Document content does not match.
      + expected - actual

       new
      -old
      -old
      +new
      +new

it fails the test

Then modify the code

-      result = this.commandList.join('');
+      return this.commandList.join('');

The running result is as follows

    `:` (command) register used as macro and command with leader key
      ✔ Repeat :s and command with leader key (44ms)
      1) Repeat :s and command with leader key
  1 failing
  1) Record and execute a macro
       `:` (command) register used as macro and command with leader key
         Repeat :s and command with leader key:

      Document content does not match.
      + expected - actual

       new
      -new
      -new
      +old
      +old

I think this should prove that I solved this bug and did not damage the original functionality

[mvandenhoek]

@mvandenhoek @J-Fields Hi, Sorry, I've been a bit busy these past few days
...
I think this should prove that I solved this bug and did not damage the original functionality

I think that if

• \n in the keysPressed of the test simulates pressing enter (I expected you'd have to write <enter> or something alone those lines, but I suppose that's just my confusion).
• The test before the change produces an error: Pattern not found: <leader>ld
• The test after the change produces the expected result of end: ['new', 'new', '|new']

Then I think that solves the issue. So if it didn't break anything else I would approve of it.

I was also trying to run the tests myself, and I could run it locally with sudo yarn test, but it runs the entire test suite and I couldn't find how to just run a single test. Did you manage to figure that out?

[HenryTSZ]

\n in the keysPressed of the test simulates pressing enter (I expected you'd have to write or something alone those lines, but I suppose that's just my confusion).

I think the existing test cases are all used \n, so there should be no problem

I was also trying to run the tests myself, and I could run it locally with sudo yarn test, but it runs the entire test suite and I couldn't find how to just run a single test. Did you manage to figure that out?

newTest -> newTestOnly

[mvandenhoek]

Looks good to me, but my knowledge of the internal workings of the vscodevim plugin is limited, so there should also be another reviewer.

[J-Fields]

Thank you!