Skip to content

Conversation

KillTheMule
Copy link
Contributor

Fixes #7244
Ref #7249

@marvim marvim added the RDY label Sep 12, 2017
@KillTheMule
Copy link
Contributor Author

Travis timed out on the linter, QB failed on ubuntu for a reason I can't discern (there does not seem to be a failed test).

@justinmk justinmk added this to the 0.2.1 milestone Sep 12, 2017
nmatch = curbuf->b_ml.ml_line_count - sub_firstlnum + 1;
skip_match = true;
}

// 3. Substitute the string. During 'inccommand' preview only do this if
// there is a replace pattern.
Copy link
Member

Choose a reason for hiding this comment

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

does that comment need to be moved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's stil in the right place I'd say, but it mainly belongs to the else if clause... Should be changed, you're right.

sub_firstline = vim_strsave((char_u *)"");
copycol = 0;
}
} else if (!preview || has_second_delim) {
Copy link
Member

Choose a reason for hiding this comment

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

instead of else-if, the usual way in this function is goto skip. That allows the structure to be more "chunked" and commentable. I'll change this in the merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, thanks.

justinmk pushed a commit to justinmk/neovim that referenced this pull request Sep 12, 2017
@justinmk
Copy link
Member

see 2736f0c

Not sure if it's better, but I guess using a macro at least gives a hint for later, if someone has a better idea in the future.

@justinmk
Copy link
Member

merged

@justinmk justinmk closed this Sep 14, 2017
@justinmk justinmk removed the RDY label Sep 14, 2017
@KillTheMule KillTheMule deleted the icm-lockfix branch September 16, 2017 18:26
nateozem pushed a commit to nateozem/neovim that referenced this pull request Oct 5, 2017
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