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

[RFC] vim-patch:7.4.310, vim-patch:7.4.313 #919

Merged
merged 2 commits into from Sep 17, 2014
Merged

Conversation

munshkr
Copy link
Contributor

@munshkr munshkr commented Jul 7, 2014

7.4.310
Problem:    getpos()/setpos() don't include curswant.
Solution:   Add a fifth number when getting/setting the cursor.

https://code.google.com/p/vim/source/detail?r=ccac0aa34eeaf46dad4b831461a532fc3fe71096

7.4.313
Problem:    Changing the return value of getpos() causes an error. (Jie Zhu)
Solution:   Revert getpos() and add getcurpos().

https://code.google.com/p/vim/source/detail?r=332a5c2b2956d9b18d85268a724d01deea27ec83

@munshkr munshkr changed the title vim-patch:7.4.310 [RFC] vim-patch:7.4.310 Jul 8, 2014
@munshkr
Copy link
Contributor Author

munshkr commented Jul 11, 2014

About these Vim patches, should we merge them while conforming to Neovim's style guide? Or should that be in another issue/PR (like a "refactor")?

@justinmk
Copy link
Member

We tend to update the style for lines that are already changing, and add missing {} braces and change /**/ to // or ///. It's a matter of judgement; a guideline is: if more than 20% of the changes are style changes, it's too noisy.

@munshkr
Copy link
Contributor Author

munshkr commented Jul 11, 2014

👍 thanks, I'll update this one then

@munshkr
Copy link
Contributor Author

munshkr commented Jul 11, 2014

Added braces and used // only on new lines. I can rebase this with neovim/docs#31 once #938 is merged.

@justinmk
Copy link
Member

@munshkr #938 is merged, so docs can be updated in this PR.

@munshkr
Copy link
Contributor Author

munshkr commented Jul 29, 2014

Rebased against master, looks like docs are already merged.

@atwupack
Copy link
Contributor

@munshkr The doc for cursor({list}) is merged but for getpos({expr}) it still needs to be done.
Edit: Just noticed that there is 7.4.313 which reverts some of the changes here and adds a new function getcurpos. Maybe we should include it here?

@munshkr
Copy link
Contributor Author

munshkr commented Jul 30, 2014

@atwupack runtime/doc was updated from Vim's master with #938, so all documentation changes from this patch and future patches (including 7.4.313) are already merged.

@atwupack
Copy link
Contributor

@munshkr I was thinking about the patch in general and not only the documentation. 7.4.313 is strongly based on 7.4.310 because in reverts some on the changes in 7.4.310. For example it would be bad if 7.4.313 got merged into master before 7.4.310. As these two patches are so strongly connected, I think that they should be part of one PR.

@munshkr
Copy link
Contributor Author

munshkr commented Jul 31, 2014

Ahh, I see what you mean. I'll include 7.4.313 in a separate commit then.

@munshkr munshkr changed the title [RFC] vim-patch:7.4.310 [RFC] vim-patch:7.4.310, vim-patch:7.4.313 Jul 31, 2014
@munshkr
Copy link
Contributor Author

munshkr commented Aug 3, 2014

@atwupack Patch 7.4.313 commited.
Also fixed test for 310, because it wasn't testing the output correctly (test.out file was being closed before writting getpos/getcurpos result).

@splinterofchaos
Copy link

I was just about to submit a PR for this when I noticed this one. @munshkr, could you rebase to the current master?

LGTM! 👍

Problem:    getpos()/setpos() don't include curswant.
Solution:   Add a fifth number when getting/setting the cursor.

https://code.google.com/p/vim/source/detail?r=ccac0aa34eeaf46dad4b831461a532fc3fe71096
Problem:    Changing the return value of getpos() causes an error.  (Jie Zhu)
Solution:   Revert getpos() and add getcurpos().

https://code.google.com/p/vim/source/detail?r=332a5c2b2956d9b18d85268a724d01deea27ec83
@munshkr
Copy link
Contributor Author

munshkr commented Sep 17, 2014

@splinterofchaos sorry for the delay... I've just rebased

@splinterofchaos
Copy link

Still LGTM. 👍

justinmk added a commit that referenced this pull request Sep 17, 2014
vim-patch:7.4.310, vim-patch:7.4.313
@justinmk justinmk merged commit 8249e4a into neovim:master Sep 17, 2014
@justinmk
Copy link
Member

LGTM too. Thanks @munshkr

@munshkr munshkr deleted the p7.4.310 branch September 17, 2014 03:30
dwb pushed a commit to dwb/neovim that referenced this pull request Feb 21, 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.

None yet

4 participants