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

Raise end-index by 1 when setting parts of Range #270

Merged
merged 2 commits into from
Oct 12, 2017

Conversation

cherti
Copy link
Contributor

@cherti cherti commented Oct 10, 2017

This commit fixes a bug in Range, leading to the last line of a Range
not being able to be replaced via __setitem__ as the final
line setting its contents in the buffer the Range is based on assumes the
end-index to be inclusive, which it isn't in Python-slicing-notation.

@justinmk

@cherti cherti changed the title Raise end-index by 1 when setting range-parts Raise end-index by 1 when setting Range-parts Oct 10, 2017
@cherti cherti changed the title Raise end-index by 1 when setting Range-parts Raise end-index by 1 when setting parts of Range Oct 10, 2017
@cherti
Copy link
Contributor Author

cherti commented Oct 11, 2017

I checked the failed checks of CI, it's flake complaints unrelated to this PR, therefore I'd say this should be ready for merge nevertheless. All tests run through.

@cherti cherti closed this Oct 11, 2017
@cherti cherti reopened this Oct 11, 2017
@bfredl
Copy link
Member

bfredl commented Oct 11, 2017

could a test be added to test/test_buffer.py ?

@cherti
Copy link
Contributor Author

cherti commented Oct 11, 2017

Certainly so. Done.

This commit fixes a bug in Range, leading to the last line of a Range
not being able to be replaced via the __setitem__-function as the final
line setting its contents in the buffer the Range is based on assumes the
end-index to be inclusive, which it isn't in Python-slicing-notation.
@cherti
Copy link
Contributor Author

cherti commented Oct 11, 2017

PR is updated, __setitem__ should now work as intended for all possible usages.

One thing that's still bugging me: I cannot reproduce a similar behavior for __getitem__, but as __getitem__ and __setitem__ are implementet analogously, I would suspect a similar issue in there.
I cannot reproduce that, though, everything looks fine for __getitem__, according to my tests.

@cherti
Copy link
Contributor Author

cherti commented Oct 12, 2017

@bfredl
ok, cannot reproduce any problems with __getitem__, just seems to be with __setitem__.

So if the test is sufficient for you, the PR is final and ready for merge.

@bfredl bfredl merged commit ace2522 into neovim:master Oct 12, 2017
bfredl added a commit to bfredl/python-client that referenced this pull request Nov 8, 2017
Brings the client up-to-date with Nvim 0.2.1

Changes since 0.1.13:
* a2e1169 Fix tests on windows (neovim#201)
* 9a0e729 fix an indexing bug when setting lines in a Range object (neovim#270)
* 4abd5d0 Documentation update (neovim#272)
* a703b47 make sure logging always uses UTF-8 regardless of locale (neovim#276)
* 68aa352 argument to allow nested notification handlers (neovim#262)
bfredl added a commit to bfredl/python-client that referenced this pull request Nov 8, 2017
Brings the client up-to-date with Nvim 0.2.1

Changes since 0.1.13:
* a2e1169 Fix tests on windows (neovim#201)
* 9a0e729 Fix an indexing bug when setting lines in a Range object (neovim#270)
* 4abd5d0 Documentation update (neovim#272)
* a703b47 Make sure logging always uses UTF-8 regardless of locale (neovim#276)
* 68aa352 Argument to allow nested notification handlers (neovim#262)
bfredl added a commit to bfredl/python-client that referenced this pull request Nov 8, 2017
Brings the client up-to-date with Nvim 0.2.1

Changes since 0.1.13:
* a2e1169 Fix tests on windows (neovim#201)
* 9a0e729 Fix an indexing bug when setting lines in a Range object (neovim#270)
* 4abd5d0 Documentation update (neovim#272)
* a703b47 Make sure logging always uses UTF-8 regardless of locale (neovim#276)
* 68aa352 Add argument to allow nested notification handlers (neovim#262)
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