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

Use norelativenumber instead of number #32

Merged
merged 2 commits into from
Jul 12, 2013
Merged

Conversation

RyanMcG
Copy link
Contributor

@RyanMcG RyanMcG commented Jul 5, 2013

  • This plugin is better for just modifying the relativenumber option since it only supports two states. The last commit removes modifications of the number option by setting norelativenumber.
  • Use s: prefixed vars for proper scope. g: means global which means other plugins can modify it. Furthermore, g:mode and g:center are not very unique names that might get used elsewhere.
  • Fixed some inconsistent tab styling. I notice some other people did this too in their pull-requests.

If you have any questions or want help with the merging or anything let me know. I'm not very handy with vim script but I'll do my best. Anyways, this plugin is a very good idea but it can be made much better if you merge the PR's currently against it.

@myusuf3
Copy link
Owner

myusuf3 commented Jul 6, 2013

@RyanMcG thanks for the pull request. theses are definite improvements. I haven't been using vim for sometime now so that might be the reason the plugin has fallen a bit behind that being said.

None of the open pull requests aside form the loading guard I just merged are solutions that I like/ had a chance to test.

That being said please fix your PR and we will have it merged! Thanks for your pull request and for fixing up inconsistencies 💯 .

@hauleth
Copy link

hauleth commented Jul 11, 2013

It works, so 👍 for merge.

@myusuf3
Copy link
Owner

myusuf3 commented Jul 11, 2013

@RyanMcG ping.

@RyanMcG
Copy link
Contributor Author

RyanMcG commented Jul 11, 2013

Thanks for putting in the effort to check it out! I hope I didn't come off too strong before, it wasn't my intention.

@myusuf3 what changes did you want me to make? Does it not merge cleanly?

@myusuf3
Copy link
Owner

myusuf3 commented Jul 11, 2013

@RyanMcG not at all. I like thoughtful criticism and improvements. :) It doesn't merge cleanly yes so if you could resolve that and test to make sure things work, we can get this merged in and look at other issues.

Really the use of the number option should be part of the plugin but
the current integration is poor. The nu and rnu means something different than
nonu and rnu for instance.
@RyanMcG
Copy link
Contributor Author

RyanMcG commented Jul 11, 2013

I pulled and rebased my fork from yours and resolved conflicts. The "conflict" was just git being sensitive and because the cpo thing was moved close to where I changed the center and mode variables scope. Does that work? It merges cleanly locally.

myusuf3 added a commit that referenced this pull request Jul 12, 2013
Use norelativenumber instead of number
@myusuf3 myusuf3 merged commit b9e6d7e into myusuf3:master Jul 12, 2013
RyanMcG added a commit to RyanMcG/dotfiles that referenced this pull request Jul 12, 2013
@RyanMcG
Copy link
Contributor Author

RyanMcG commented Jul 12, 2013

My VIM version:

VIM - Vi IMproved 7.3 (2010 Aug 15, compiled Jul  2 2013 18:28:42)
Included patches: 1-1287
Compiled by Arch Linux

Here is a snippet from the vim help I acquired by doing locally :h number_relativenumber.

                                            *number_relativenumber*
    The 'relativenumber' option changes the displayed number to be
    relative to the cursor.  Together with 'number' there are these
    four combinations (cursor in line 3):

                'nonu'          'nu'            'nonu'          'nu'
                'nornu'         'nornu'         'rnu'           'rnu'

        |apple          |  1 apple      |  2 apple      |  2 apple
        |pear           |  2 pear       |  1 pear       |  1 pear
        |nobody         |  3 nobody     |  0 nobody     |3   nobody
        |there          |  4 there      |  1 there      |  1 there

After a bit of digging I've found that the change occurs at patch 1115 (help text updated here). So versions 7.3.1115 and greater have these 4 various states instead of the previous 3 (number, relativenumber, nonumber) while in versions below that the relativenumber option resets number. Here is the google group topic discussing the change.

We need to have the plugin behave differently depending on the version. In effect this PR fixes this plugin for versions 7.3.1115 and greater and breaks it for older versions. I intend to submit another PR fixing this for both parties 😄.

It's important to note that with 4 different states we can't model all possibilities with s:mode being a zero or a one. I'll address this in my upcoming PR as well.

@hauleth
Copy link

hauleth commented Jul 13, 2013

Or make second branch for users of old Vim patches.

@myusuf3
Copy link
Owner

myusuf3 commented Jul 13, 2013

@hauleth pull down newest master. let me know if this fixes things for you.

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.

5 participants