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

Vim mode word selection is off by one (affecting diw/daw/diW/daW) #1503

Closed
wants to merge 2 commits into from

Conversation

abrooks
Copy link
Contributor

@abrooks abrooks commented May 6, 2013

There are several variants of the bug, but, most basically, if you have this text and are sitting on "A":

foo bAr baz

And you execute diw, you'll get:

foo r baz

Instead of:

foo baz

This issue affects all of the following delete motions: diw, daw, diW, daW as well as their change (ciw...), yank (yiw...) and visual (viw...) variations.

I created 28 tests covering various edge cases (26 cases fail without the patch) and all now pass. lint was happy as well.

Let me know if there's anything you'd like me to change — style, completeness or otherwise.

Thanks!

@mightyguava
Copy link
Contributor

I'd been hoping someone would take a serious crack at fixing text objects. This is one of the best patches I've seen so far. Great job and thanks for the unit tests!

The patch mostly looks good, except that I think you need to also consider words separated by multiple spaces? Your patch seems to only take into account single spaces.

@abrooks
Copy link
Contributor Author

abrooks commented May 6, 2013

I'd be happy to. Give me a few minutes or so and I'll push again.

@abrooks
Copy link
Contributor Author

abrooks commented May 6, 2013

@mightyguava Good call. Vim deletes all whitespace trailing or preceding for the "a" inclusive motions. This will take longer as I adjust the code. I'll update the pull request with the new changes when they're done.

@abrooks
Copy link
Contributor Author

abrooks commented May 6, 2013

This updated push addresses both the multi-whitespace tests and the fixed logic to handle the multi-whitespace inclusion. Really, the logic reads more like the Vim behavior now anyways.

Let me know if there are more tests or other changes to the commits you'd like to see.

@mightyguava
Copy link
Contributor

Looks good to me. Thank you for taking the time to make these changes. I really appreciate your attention to detail. Lots of people will be happy to see these fixes :)

@abrooks
Copy link
Contributor Author

abrooks commented May 6, 2013

Thanks! I really appreciate the project and hope to contribute again. :)

@marijnh
Copy link
Member

marijnh commented May 6, 2013

Thanks! Merged.

@marijnh marijnh closed this May 6, 2013
@lynschinzer
Copy link
Contributor

Thanks +1, this patch is seriously awesome!

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.

None yet

4 participants