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

Merged
merged 22 commits into from
Jun 1, 2025
Merged

Implements vimgrep, #5991 #9630

merged 22 commits into from
Jun 1, 2025

Conversation

AzimovParviz
Copy link
Contributor

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
Contributor Author

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

this is how it looks like as of now

@AzimovParviz
Copy link
Contributor 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
Contributor 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
Contributor 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.

@AzimovParviz
Copy link
Contributor Author

I cannot get the tests to work anymore, so I've been trying to figure out what is wrong. It seems that the focus does not happen anymore inside the test. Is there a way to run a singular test on bare metal to visually assert for me what is actually happening?

@AzimovParviz
Copy link
Contributor Author

I was too lazy to figure that out and just a put a 2 minute timeout in the test right before the assertions. The problem was with the file paths you get from the testUtils's createFile (they were very long due to being run from inside the Extension test host VSCode I suppose and it seems to have thrown the VSCode's search off).

@AzimovParviz AzimovParviz requested a review from J-Fields May 31, 2025 17:28
Copy link
Member

@J-Fields J-Fields left a comment

Choose a reason for hiding this comment

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

Thanks!

@J-Fields J-Fields merged commit 3786a02 into VSCodeVim:master Jun 1, 2025
1 check passed
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