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.

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

Replace mode #580

Merged
merged 5 commits into from
Aug 8, 2016
Merged

Replace mode #580

merged 5 commits into from
Aug 8, 2016

Conversation

rebornix
Copy link
Member

@rebornix rebornix commented Aug 8, 2016

Yay! We love PRs! 🎊

Please include a description of your change & check your PR against this list, thanks:

  • Commit message has a short title & issue references
  • Each commit does a logical chunk of work.
  • It builds and tests pass (e.g gulp tslint)

Add a new mode; Replace :)

constructor(startPosition: Position) {
this._replaceCursorStartPosition = startPosition;
let text = TextEditor.getLineAt(startPosition).text.substring(startPosition.character);
/* tslint:disable:forin */
Copy link
Member

Choose a reason for hiding this comment

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

Use for of

@johnfn
Copy link
Member

johnfn commented Aug 8, 2016

Whoa, this is huge! Replace mode has been a big want of mine. Is it ready to go?

@rebornix
Copy link
Member Author

rebornix commented Aug 8, 2016

@johnfn replace for...in with for...of with returning iterators. I suppose it's ready to go :)

constructor(startPosition: Position) {
this._replaceCursorStartPosition = startPosition;
let text = TextEditor.getLineAt(startPosition).text.substring(startPosition.character);
for (let [key, value] of text.split("").entries()) {
Copy link
Member

Choose a reason for hiding this comment

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

That's that ES6 style that I like :-)

@johnfn
Copy link
Member

johnfn commented Aug 8, 2016

Yeah, it generally looks good to me. Two quick questions before merge.

  • Can you add some tests?
  • Is it repeatable with .? If so, can you test that as well?

@rebornix
Copy link
Member Author

rebornix commented Aug 8, 2016

@johnfn my bad ... I'll add tests real quick.

@rebornix
Copy link
Member Author

rebornix commented Aug 8, 2016

Added test cases for Replace Mode and . repeat.

The coverage of . repeat might be low but it now covers most user cases.

@rebornix
Copy link
Member Author

rebornix commented Aug 8, 2016

Replace commands can be prefixed with numbers but it's not implemented here, if I implement it before this PR gets merged, I'll update this PR. Otherwise create separate task to track it.

@johnfn
Copy link
Member

johnfn commented Aug 8, 2016

Wonderful work, @rebornix. This is big enough to push out a new version, IMO. I'll give it a spin locally and hopefully get to doing that later tonight. :)

@johnfn johnfn merged commit f5885a6 into VSCodeVim:master Aug 8, 2016
This pull request was closed.
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.

2 participants