Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow regex in / search #627

Merged
merged 6 commits into from
Aug 28, 2016
Merged

Allow regex in / search #627

merged 6 commits into from
Aug 28, 2016

Conversation

ascandella
Copy link
Contributor

@ascandella ascandella commented Aug 19, 2016 •
edited
Loading

Fixes #572

@johnfn
Copy link
Member

johnfn commented Aug 22, 2016

My big concern is I'm not quite sure how well Vim regexp maps to JS regexp. Would you mind doing a little research to see if it is a fairly close mapping or not?

@ascandella
Copy link
Contributor Author

Sounds good. Hopefully they're close...

@johnfn
Copy link
Member

johnfn commented Aug 22, 2016

I hope so too. Or close enough that you can use a meta regex to translate the normal regex to a js re... oh god, what am I saying?

@rebornix
Copy link
Member

@sectioneight the regex we use here is the same as Vim Substitution

Vim/src/cmd_line/commands/substitute.ts

Line 64 in 7ac106d

let jsRegexFlags = "";
. The catch about Regex is no one is following the standard (if there is a standard).

For now I'm using JS's regex in substitution, I'm not quite sure if ppl are satisfied with it but I don't see any complains yet (maybe they don't notice its existence yet).

You may want to write some JS regex to translate Vim's regex to JS's regex ...

@ascandella
Copy link
Contributor Author

Yes, I've found:

https://github.com/othree/eregex.vim/blob/master/plugin/eregex.vim

To translate Vim regex to PCRE, but of course JS is not PCRE.

There's also https://en.wikipedia.org/wiki/Comparison_of_regular_expression_engines but it's pretty high level.

Sounds like a good first pass would be to use JS regex for search. But I'm open to suggestions.

@johnfn
Copy link
Member

johnfn commented Aug 22, 2016

Yiiiikes. If it's too complicated, let's just do JS regex for now and see if anyone complains. 😉

@ascandella ascandella force-pushed the support-search-regex branch from 7eeb859 to e31a63b Compare August 25, 2016 18:43
@rebornix
Copy link
Member

This PR is good to go IMO.

@johnfn
Copy link
Member

johnfn commented Aug 26, 2016

It still says WIP on it - @sectioneight is this good to go?

@ascandella
Copy link
Contributor Author

Still need to add a test or two, but it sounds like you guys approve of the approach.

@johnfn
Copy link
Member

johnfn commented Aug 26, 2016 •
edited
Loading

I do; I'm very much a fan of 80% solutions, especially when people very rarely use the other 20%. Ping me when it's good to go. :)

@ascandella
Copy link
Contributor Author

As am I. Will get some tests in tomorrow and ping you for review.
On Thu, Aug 25, 2016 at 10:35 PM Grant Mathews notifications@github.com
wrote:

I do; I'm very much a fan of 80% solutions, especially when people very
rarely use the other 20%.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#627 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAF7PwxoDZRo1_dE7y_86vz1fYnKPNKJks5qjnsGgaJpZM4JojVl
.

@ascandella ascandella force-pushed the support-search-regex branch from e31a63b to 343779b Compare August 26, 2016 18:11
@ascandella ascandella changed the title WIP: Allow regex in / search Allow regex in / search Aug 26, 2016
@ascandella ascandella force-pushed the support-search-regex branch from 343779b to bcaeba6 Compare August 26, 2016 18:12
@ascandella
Copy link
Contributor Author

Ok, ready for review @johnfn

johnfn
johnfn reviewed Aug 26, 2016
View reviewed changes
src/actions/actions.ts
@@ -782,7 +782,7 @@ export class CommandSearchForwards extends BaseCommand {
isMotion = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
vimState.searchState = new SearchState(SearchDirection.Forward, vimState.cursorPosition);
vimState.searchState = new SearchState(SearchDirection.Forward, vimState.cursorPosition, "", true);
Copy link
Member

Choose a reason for hiding this comment

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

Minor, but I'm trying to move towards a style where we explicitly name all our boolean arguments. Something like { isRegex: true } is what I'm thinking here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good.

@johnfn
Copy link
Member

johnfn commented Aug 26, 2016

Only one small comment.

I think this PR is going to have a big impact for such a small LOC difference! :)

@ascandella
Copy link
Contributor Author

I'm going to update one more thing on this PR. Right now if you have a partial regex (e.g. /a() it throws an error in the console. I need to try/catch the SyntaxError and just do a plain match if it doesn't compile.

ascandella
ascandella reviewed Aug 26, 2016
View reviewed changes
src/mode/modeHandler.ts
@@ -280,10 +295,11 @@ export class SearchState {
}
}

constructor(direction: SearchDirection, startPosition: Position, searchString = "") {
constructor(direction: SearchDirection, startPosition: Position, searchString = "", { isRegex = false } = {}) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really sure about this syntax (still a TS noob). Is this what you were thinking?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that looks fine to me!

@ascandella
Copy link
Contributor Author

OK, I think we're finally ready to go here

@ascandella
Copy link
Contributor Author

Ping for review

johnfn
johnfn reviewed Aug 28, 2016
View reviewed changes
src/mode/modeHandler.ts
const regex = new RegExp(search.replace(SearchState.specialCharactersRegex, "\\$&"), ignorecase ? 'gi' : 'g');
let searchRE = search;
if (!this.isRegex) {
searchRE = search.replace(SearchState.specialCharactersRegex, "\\$&");
Copy link
Member

Choose a reason for hiding this comment

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

This is very cool. I never knew about $&.

@johnfn
Copy link
Member

johnfn commented Aug 28, 2016

Thanks for the ping. Feel free to ping me on here/slack if I don't review within ~24h. That is almost certainly an indicator that it just slipped through the cracks.

@johnfn johnfn merged commit de982ac into VSCodeVim:master Aug 28, 2016
@johnfn
Copy link
Member

johnfn commented Aug 28, 2016

And thanks for the PR, of course :)

@ascandella ascandella deleted the support-search-regex branch August 28, 2016 19:20
@shaunluttin
Copy link

shaunluttin commented Feb 5, 2017 •
edited
Loading

Does this PR support searching for new lines? If so, how? We've tried:

  • \r
  • \n
  • \n\r
  • ^M

@simahao
Copy link

simahao commented Apr 16, 2017

@shaunluttin ,i have same problem. :(

@johnfn
Copy link
Member

johnfn commented Apr 16, 2017

No, it doesn't support newline search yet. Feel free to open an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ascandella @johnfn @rebornix @shaunluttin @simahao