Skip to content

Implements vimgrep, #5991 #9630

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Conversation

AzimovParviz
Copy link

What this PR does / why we need it:
Adds a basic implementation of vimgrep, which doesn't reinvent the wheel and uses native VSCode search function
Which issue(s) this PR fixes
#5991

Special notes for your reviewer:
I still haven't thought of how to implement the E480 error for "no match".
Also flags like g and j are still missing. while j is implementable, I am not sure how to limit VS Code's built-in search results by a number

@AzimovParviz
Copy link
Author

Screen.Recording.2025-05-17.at.22.56.20.mov

this is how it looks like as of now

@AzimovParviz
Copy link
Author

@J-Fields I apologize for pinging. What is the bare minimum that would be expected from a vimgrep implementation?

And an off-topic question, does the VSCodeVim Slack channel still exist? I would love to ask more questions from contributors and maybe get some advice as well.

Thank you in advance

@J-Fields
Copy link
Member

What is the bare minimum that would be expected from a vimgrep implementation?

I think what you've got here is merge-able, in terms of features.

And an off-topic question, does the VSCodeVim Slack channel still exist? I would love to ask more questions from contributors and maybe get some advice as well.

Not in any meaningful way unfortunately. At the moment I'm the only active maintainer (and that's being very loose with the term "active"). Are there are any particular questions you have?

@AzimovParviz AzimovParviz marked this pull request as ready for review May 23, 2025 03:44
@AzimovParviz
Copy link
Author

Are there are any particular questions you have?

I had some questions about writing tests (I was confused on how to write them at all. I was wondering, for example, if it was required to use the test simplifier. I wasn't able to figure out on how to include it in my tests properly) and I was also wondering about some minor things like the command strings in the command parser.

All of these got resolved now, although it's a bit saddening that the Slack is not active. Thank you for maintaining the repo, it's my favorite Vim mode implementation across different text editors and IDEs

@J-Fields
Copy link
Member

I had some questions about writing tests (I was confused on how to write them at all. I was wondering, for example, if it was required to use the test simplifier.

For cases where side-effects you care about are within the editor, it's preferred. But for something like this, where we're opening a native VSCode panel, the simplifier isn't gonna cut it.

I was also wondering about some minor things like the command strings in the command parser.

Can you clarify what you mean?

Thank you for maintaining the repo, it's my favorite Vim mode implementation across different text editors and IDEs

Thanks, happy to hear it!

@AzimovParviz
Copy link
Author

Can you clarify what you mean?

I was a little confused on why the commands were parsed like 'gr' 'ep', 'fu' 'nction' etc. I tried reading the vim manual and the commands didn't look like that in there.

@AzimovParviz AzimovParviz requested a review from J-Fields May 28, 2025 02:59
@mikaelmedina
Copy link

I was a little confused on why the commands were parsed like 'gr' 'ep', 'fu' 'nction' etc. I tried reading the vim manual and the commands didn't look like that in there.

My guess is that the first part in the tuple is the shorthand alias that works, i.e. 'gr' is unique so we can shorthand it and use :gr pattern file, then the second part expands it to what the full command would be. Similar to how in .vimrc you can use set nu instead of set number as nu is unique already.

If this is correct the I guess the entry for [['grep', ''], GrepCommand.argParser], is strictly unnecessary. (?)

Also this looks like a nice way to get this feature going, nice to embrace the native VScode features sometimes. 👍

@J-Fields
Copy link
Member

My guess is that the first part in the tuple is the shorthand alias that works

That's correct - this allows us to specify all the equivalent commands in one line.

If this is correct the I guess the entry for [['grep', ''], GrepCommand.argParser], is strictly unnecessary. (?)

Yep - I must've missed that in my first read.

@@ -577,7 +578,8 @@ export const builtinExCommands: ReadonlyArray<[[string, string], ArgParser | und
[['vert', 'ical'], undefined],
[['vi', 'sual'], undefined],
[['vie', 'w'], undefined],
[['vim', 'grep'], undefined],
[['vim', 'grep'], GrepCommand.argParser],
[['grep', ''], GrepCommand.argParser],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[['grep', ''], GrepCommand.argParser],

Comment on lines +10 to +26
// This will go into the exCommandParse test
// function exParseTest(input: string, parsed: ExCommand) {
// test(input, () => {
// const { command } = exCommandParser.tryParse(input);
// assert.deepStrictEqual(command, parsed);
// });
// }

// suite('grep', () => {
// const pattern = Pattern.parser({ direction: SearchDirection.Forward, delimiter: '/' });
// exParseTest(':vimgrep "t*st" foo.txt',
// new GrepCommand({
// pattern: pattern.tryParse('t*st'),
// files: ['foo.txt'],
// }),
// );
// });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go in exCommandParse.test.ts, I think

Comment on lines +35 to +43
test('GrepCommand parses correctly', async () => {
await vscode.commands.executeCommand('workbench.action.files.newUntitledFile');
const pattern = Pattern.parser({ direction: SearchDirection.Backward, delimiter: '/' });
const command = grep(pattern.tryParse('t*st'), ['Untitled-1']);
assert.deepStrictEqual(command.arguments, {
pattern: pattern.tryParse('t*st'),
files: ['Untitled-1'],
});
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, Add a few test cases to exCommandParse.test.ts and delete this

// it will also be in normal mode if you run vimgrep from another file
test('GrepCommand executes correctly', async () => {
// Untitled-1
await vscode.commands.executeCommand('workbench.action.files.newUntitledFile');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use testUtils.createFile instead. You can pass it the contents directly and since it'll be an already-named file on disk, you won't have to worry about the save confirmation below.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants