Fixed off by one error in operating on text objects #1370

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@RyanJenkins

I'm a little nervous about this because it involved removing part of a line whose reason for existing wasn't clear. There doesn't seem to be any unwanted side effects, but it seems strange the the line existed in the first place if all it did was cause this error.

Fixed off by one error on text objects
Also changed expandWordUnderCursor to select adjacent consecutive
whitespece when the cursor is on a whitespace character. This is the
default behaviour in Vim 7.2 and GVim 7.3.
@mightyguava

This comment has been minimized.

Show comment Hide comment
@mightyguava

mightyguava Mar 16, 2013

Contributor

I see what you are trying to do with the whitespace changes, but this is shared logic for # and * and seems to break the tests for # and *. Maybe instead of doing this, add in preceding whitespace in the text object manipulation logic after calling expandWordUnderCursor?

As for the var wordEnd = idx + wordAfterRegex[0].length - 1;, this was based on char index rather than bounds, so wordEnd is index of the last char, not the right bound for the word. I'm ok with this change except that you'll need to remove the + 1 in var query = cm.getLine(word.start.line).substring(word.start.ch, word.end.ch + 1); in the search logic. Please test for additional breakages.

Contributor

mightyguava commented Mar 16, 2013

I see what you are trying to do with the whitespace changes, but this is shared logic for # and * and seems to break the tests for # and *. Maybe instead of doing this, add in preceding whitespace in the text object manipulation logic after calling expandWordUnderCursor?

As for the var wordEnd = idx + wordAfterRegex[0].length - 1;, this was based on char index rather than bounds, so wordEnd is index of the last char, not the right bound for the word. I'm ok with this change except that you'll need to remove the + 1 in var query = cm.getLine(word.start.line).substring(word.start.ch, word.end.ch + 1); in the search logic. Please test for additional breakages.

@abrooks

This comment has been minimized.

Show comment Hide comment
@abrooks

abrooks May 6, 2013

Contributor

I've provided a working patch with tests in pull request #1503 that addresses this issue. If that item is pulled, this one can be closed.

Contributor

abrooks commented May 6, 2013

I've provided a working patch with tests in pull request #1503 that addresses this issue. If that item is pulled, this one can be closed.

@marijnh marijnh closed this May 6, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment